[patch] gcn: Add __builtin_gcn_kernarg_ptr

2022-11-16 Thread Tobias Burnus

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?

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.
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
gcn: Add __builtin_gcn_kernarg_ptr

Add __builtin_gcn_kernarg_ptr to avoid using hard-coded register values
and permit future ABI changes while keeping the API.

gcc/ChangeLog:

* config/gcn/gcn-builtins.def (KERNARG_PTR): Add.
* config/gcn/gcn.cc (gcn_init_builtin_types): Change siptr_type_node,
	sfptr_type_node and voidptr_type_node from FLAT to ADDR_SPACE_DEFAULT.
(gcn_expand_builtin_1): Handle GCN_BUILTIN_KERNARG_PTR.
(gcn_oacc_dim_size): Return in ADDR_SPACE_FLAT.

libgomp/ChangeLog:

* config/gcn/team.c (gomp_gcn_enter_kernel): Use
	__builtin_gcn_kernarg_ptr instead of asm ("s8").

Co-Authored-By: Andrew Stubbs 

 gcc/config/gcn/gcn-builtins.def |  4 
 gcc/config/gcn/gcn.cc   | 24 
 libgomp/config/gcn/team.c   |  2 +-
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/gcc/config/gcn/gcn-builtins.def b/gcc/config/gcn/gcn-builtins.def
index c50777bd..eeeaebf 100644
--- a/gcc/config/gcn/gcn-builtins.def
+++ b/gcc/config/gcn/gcn-builtins.def
@@ -158,6 +158,10 @@ DEF_BUILTIN (ACC_SINGLE_COPY_END, -1, "single_copy_end", B_INSN,
 DEF_BUILTIN (ACC_BARRIER, -1, "acc_barrier", B_INSN, _A1 (GCN_BTI_VOID),
 	 gcn_expand_builtin_1)
 
+/* Kernel inputs.  */
+
+DEF_BUILTIN (KERNARG_PTR, -1, "kernarg_ptr", B_INSN, _A1 (GCN_BTI_VOIDPTR),
+	 gcn_expand_builtin_1)
 
 #undef _A1
 #undef _A2
diff --git a/gcc/config/gcn/gcn.cc b/gcc/config/gcn/gcn.cc
index 5e6f3b8..b3814c2 100644
--- a/gcc/config/gcn/gcn.cc
+++ b/gcc/config/gcn/gcn.cc
@@ -4058,15 +4058,15 @@ gcn_init_builtin_types (void)
 	  (integer_type_node) */
 	, 64);
   tree tmp = build_distinct_type_copy (intSI_type_node);
-  TYPE_ADDR_SPACE (tmp) = ADDR_SPACE_FLAT;
+  TYPE_ADDR_SPACE (tmp) = ADDR_SPACE_DEFAULT;
   siptr_type_node = build_pointer_type (tmp);
 
   tmp = build_distinct_type_copy (float_type_node);
-  TYPE_ADDR_SPACE (tmp) = ADDR_SPACE_FLAT;
+  TYPE_ADDR_SPACE (tmp) = ADDR_SPACE_DEFAULT;
   sfptr_type_node = build_pointer_type (tmp);
 
   tmp = build_distinct_type_copy (void_type_node);
-  TYPE_ADDR_SPACE (tmp) = ADDR_SPACE_F

Re: [patch] gcn: Add __builtin_gcn_kernarg_ptr

2022-11-16 Thread Andrew Stubbs

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.