Re: [patch] gcn: Add __builtin_gcn_kernarg_ptr

[Andrew Stubbs <ams@codesourcery.com>]

On 16/11/2022 11:42, Tobias Burnus wrote:
> This is a part of a patch by Andrew (hi!) - namely that part that only 
> adds the
> __builtin_gcn_kernarg_ptr. More is planned, see below.
> 
> The short term benefit of this patch is to permit replacing hardcoded 
> numbers
> by a builtin – like in libgomp (see patch) or in newlib (not submitted):
> 
> --- a/newlib/libc/sys/amdgcn/write.c
> +++ b/newlib/libc/sys/amdgcn/write.c
> @@ -59,1 +59,5 @@ _READ_WRITE_RETURN_TYPE write (int fd, const void 
> *buf, size_t count)
> +#if defined(__has_builtin) && __has_builtin(__builtin_gcn_kernarg_ptr)
> +  register void **kernargs = __builtin_gcn_kernarg_ptr ();
> +#else
>     register void **kernargs asm("s8");
> +#endif
> 
> It would also replace the 'asm("s8")' in reverse offload (GCN) patch, i.e.
> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602339.html
> 
> 
> However, this patch is only the very first step. Next one is to add
> several additional builtins, namely those that are required for newlib,
> i.e.  newlib/libc/machine/amdgcn/mlock.c (sbrk) and
> newlib/libc/machine/amdgcn/getreent.c (__getreent) use some additional
> hard-coded value for heap and stack memory.
> 
> And at some point - but only after newlib has been updated -
> we can think of making stack variables non-private.
> That's a general goal - and in any case required for reverse
> offload to be able to transfer between the host and on-device stack
> variables.
> 
> * * *
> 
> Regarding the patch: Besides the obvious change (addition of the builtin),
> the change to DEFAULT memory space is required to avoid a memory-space 
> conversion
> ICE when using the new builtin. The gcn_oacc_dim_size change is mainly just
> picked from Andrew's patch as it seems to be reasonable. In terms of the 
> libgomp
> testsuite, I did not spot anything except that the -O2 run now does no 
> longer fail
> with "libgomp: target function wasn't mapped" for
> libgomp.oacc-fortran/kernels-map-1.f90 - but I am not sure it is related 
> or not.
> 
> In any case, the libgomp testsuite shows no fails (but the usual fails)
> with the attached patch.
> 
> OK for mainline?

OK.

Andrew

> Tobias
> 
> PS: The plan is to have at least all builtins in GCC and use them in 
> newlib by at
> the end of this year (i.e. in newlib's end of year snapshot - aka as annual
> release).
> 
> PPS: I wonder whether
>    [Patch] libgomp/gcn: Prepare for reverse-offload callback handling
>    https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602339.html
> would be okay after this patch - with the asm("s8") replaced by the 
> builtin - or not.
> The code itself would be fine, but it is unreachable until
> GOMP_OFFLOAD_get_num_devices accepts reverse offload and the latter depends
> on the support for non-private stack variables.