[Patch][AMD GCN][OpenMP] Add gcc/config/gcn/t-omp-device for OpenMP declare variant kind/arch/isa

[Patch][AMD GCN][OpenMP] Add gcc/config/gcn/t-omp-device for OpenMP declare variant kind/arch/isa

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

This adds gcc/config/gcn/t-omp-device to augment the nvptx, hsa and x86 
host implementation.

This compiler has -march= carrizo/fiji/gfx900/gfx906 (for Corrizo, Fiji 
and Vega GPUs; arch names aligned with LLVM and passed on to the linker).

This patch uses kind = "gnu" and those -march names for arch *and* isa, 
but can argue about this choice.

(I have build the GCN compiler, but didn't do any additional tests – 
also because GCN as offloading compiler is not yet supported on the trunk.)

OK for the trunk?

Tobias


[-- Attachment #2: gcn-omp.diff --]
[-- Type: text/x-patch, Size: 4346 bytes --]

2019-11-04  Tobias Burnus  <tobias@codesourcery.com>

	gcc/
	* config/gcn/gcn.c (gcn_omp_device_kind_arch_isa): New function.
	(TARGET_OMP_DEVICE_KIND_ARCH_ISA): Redefine to
	gcn_omp_device_kind_arch_isa.
	* config/gcn/t-omp-device: New file.
	* configure.ac: Support gcn for omp_device_property.
	* configure: Regenerate.



diff --git a/gcc/config/gcn/gcn.c b/gcc/config/gcn/gcn.c
index b5f09da173c..21219877431 100644
--- a/gcc/config/gcn/gcn.c
+++ b/gcc/config/gcn/gcn.c
@@ -2516,6 +2516,35 @@ gcn_gimplify_va_arg_expr (tree valist, tree type,
   return t;
 }
 
+/* Return 1 if TRAIT NAME is present in the OpenMP context's
+   device trait set, return 0 if not present in any OpenMP context in the
+   whole translation unit, or -1 if not present in the current OpenMP context
+   but might be present in another OpenMP context in the same TU.  */
+
+int
+gcn_omp_device_kind_arch_isa (enum omp_device_kind_arch_isa trait,
+			      const char *name)
+{
+  switch (trait)
+    {
+    case omp_device_kind:
+      return strcmp (name, "gpu") == 0;
+    case omp_device_arch:
+    case omp_device_isa:
+      if (strcmp (name, "carrizo") == 0)
+	return gcn_arch == PROCESSOR_CARRIZO;
+      if (strcmp (name, "fiji") == 0)
+	return gcn_arch == PROCESSOR_FIJI;
+      if (strcmp (name, "gfx900") == 0)
+	return gcn_arch == PROCESSOR_VEGA;
+      if (strcmp (name, "gfx906") == 0)
+	return gcn_arch == PROCESSOR_VEGA;
+      return 0;
+    default:
+      gcc_unreachable ();
+    }
+}
+
 /* Calculate stack offsets needed to create prologues and epilogues.  */
 
 static struct machine_function *
@@ -6030,6 +6059,8 @@ print_operand (FILE *file, rtx x, int code)
 #define TARGET_FUNCTION_VALUE_REGNO_P gcn_function_value_regno_p
 #undef  TARGET_GIMPLIFY_VA_ARG_EXPR
 #define TARGET_GIMPLIFY_VA_ARG_EXPR gcn_gimplify_va_arg_expr
+#undef TARGET_OMP_DEVICE_KIND_ARCH_ISA
+#define TARGET_OMP_DEVICE_KIND_ARCH_ISA gcn_omp_device_kind_arch_isa
 #undef  TARGET_GOACC_ADJUST_PROPAGATION_RECORD
 #define TARGET_GOACC_ADJUST_PROPAGATION_RECORD \
   gcn_goacc_adjust_propagation_record
diff --git a/gcc/config/gcn/t-omp-device b/gcc/config/gcn/t-omp-device
new file mode 100644
index 00000000000..c79dc4b83dd
--- /dev/null
+++ b/gcc/config/gcn/t-omp-device
@@ -0,0 +1,4 @@
+omp-device-properties-gcn: $(srcdir)/config/gcn/gcn.c
+	echo kind: gpu > $@
+	echo arch: carrizo fiji gfx900 gfx906 >> $@
+	echo isa: carrizo fiji gfx900 gfx906 >> $@
diff --git a/gcc/configure b/gcc/configure
index 6808c23d26b..a2df82e021f 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -7896,6 +7896,10 @@ for tgt in `echo $enable_offload_targets | sed 's/,/ /g'`; do
 	omp_device_property=omp-device-properties-i386
 	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/i386/t-omp-device"
 	;;
+      gcn*-*)
+	omp_device_property=omp-device-properties-gcn
+	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/gcn/t-omp-device"
+	;;
       nvptx*-*)
 	omp_device_property=omp-device-properties-nvptx
 	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/nvptx/t-omp-device"
@@ -18933,7 +18937,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 18936 "configure"
+#line 18940 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -19039,7 +19043,7 @@ else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 19042 "configure"
+#line 19046 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 1a0d68208e4..5f32fd4d5e4 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -1037,6 +1037,10 @@ for tgt in `echo $enable_offload_targets | sed 's/,/ /g'`; do
 	omp_device_property=omp-device-properties-i386
 	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/i386/t-omp-device"
 	;;
+      gcn*-*)
+	omp_device_property=omp-device-properties-gcn
+	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/gcn/t-omp-device"
+	;;
       nvptx*-*)
 	omp_device_property=omp-device-properties-nvptx
 	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/nvptx/t-omp-device"

Re: [Patch][AMD GCN][OpenMP] Add gcc/config/gcn/t-omp-device for OpenMP declare variant kind/arch/isa

[Jakub Jelinek]

On Mon, Nov 04, 2019 at 04:31:10PM +0100, Tobias Burnus wrote:
> This adds gcc/config/gcn/t-omp-device to augment the nvptx, hsa and x86 host
> implementation.
> 
> This compiler has -march= carrizo/fiji/gfx900/gfx906 (for Corrizo, Fiji and
> Vega GPUs; arch names aligned with LLVM and passed on to the linker).
> 
> This patch uses kind = "gnu" and those -march names for arch *and* isa, but

You mean "gpu" ;)

> can argue about this choice.

My preference would be that arch on amdgcn is something like amdgcn or gcn.
I hope the general distinction between arch and isa will be something that
will be discussed next Tuesday on the language committee, so hopefully we'll
know more afterwards and can tweak afterwards depending on the outcome.

Ok with that change.

	Jakub

Re: [Patch][AMD GCN][OpenMP] Add gcc/config/gcn/t-omp-device for OpenMP declare variant kind/arch/isa

[Andrew Stubbs]

On 04/11/2019 15:37, Jakub Jelinek wrote:
> My preference would be that arch on amdgcn is something like amdgcn or gcn.
> I hope the general distinction between arch and isa will be something that
> will be discussed next Tuesday on the language committee, so hopefully we'll
> know more afterwards and can tweak afterwards depending on the outcome.
> 
> Ok with that change.

I'm fine with this, too. The OpenMP support should be posted Real Soon Now.

We've not been terribly consistent with "gcn" vs. "amdgcn", which is 
unfortunate, but we are where we are. Most of the API enums have "GCN", 
so let's use that, for now.

Andrew

Re: [Patch][AMD GCN][OpenMP] Add gcc/config/gcn/t-omp-device for OpenMP declare variant kind/arch/isa

[Jakub Jelinek]

On Mon, Nov 04, 2019 at 04:33:27PM +0000, Andrew Stubbs wrote:
> On 04/11/2019 15:37, Jakub Jelinek wrote:
> > My preference would be that arch on amdgcn is something like amdgcn or gcn.
> > I hope the general distinction between arch and isa will be something that
> > will be discussed next Tuesday on the language committee, so hopefully we'll
> > know more afterwards and can tweak afterwards depending on the outcome.
> > 
> > Ok with that change.
> 
> I'm fine with this, too. The OpenMP support should be posted Real Soon Now.
> 
> We've not been terribly consistent with "gcn" vs. "amdgcn", which is
> unfortunate, but we are where we are. Most of the API enums have "GCN", so
> let's use that, for now.

With the way it is defined in the OpenMP spec, there can be actually aliases, so
having both gcn and amdgcn as supported arch is fine too.

	Jakub

Re: [Patch][AMD GCN][OpenMP] Add gcc/config/gcn/t-omp-device for OpenMP declare variant kind/arch/isa

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 3387 bytes --]

Hi!

On 2019-11-04T17:35:32+0100, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Nov 04, 2019 at 04:33:27PM +0000, Andrew Stubbs wrote:
>> On 04/11/2019 15:37, Jakub Jelinek wrote:
>> > My preference would be that arch on amdgcn is something like amdgcn or gcn.
>> > I hope the general distinction between arch and isa will be something that
>> > will be discussed next Tuesday on the language committee, so hopefully we'll
>> > know more afterwards and can tweak afterwards depending on the outcome.
>> >
>> > Ok with that change.
>>
>> I'm fine with this, too. The OpenMP support should be posted Real Soon Now.
>>
>> We've not been terribly consistent with "gcn" vs. "amdgcn", which is
>> unfortunate, but we are where we are. Most of the API enums have "GCN", so
>> let's use that, for now.
>
> With the way it is defined in the OpenMP spec, there can be actually aliases, so
> having both gcn and amdgcn as supported arch is fine too.

Has this been resolved yet?


On 2019-11-04T16:31:10+0100, Tobias Burnus <tobias@codesourcery.com> wrote:
> This adds gcc/config/gcn/t-omp-device to augment the nvptx, hsa and x86
> host implementation.

> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -1037,6 +1037,10 @@ for tgt in `echo $enable_offload_targets | sed 's/,/ /g'`; do

> +      gcn*-*)
> +     omp_device_property=omp-device-properties-gcn
> +     omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/gcn/t-omp-device"
> +     ;;

After some messy debugging (see the commit log), I've pushed as obvious
to master branch in commit b6a0ae1d22c9675f4374c2cb2b5c0833bb1461f1
"[gcn] Fix 'omp-device-properties-gcn' handling", see attached.

Quoting '"$${props}"' as a safety measure didn't solve the problem, as
we're then just being told 'sed: can't read : No such file or directory'
thrice, without any error condition arising -- sloppy shell programming
without proper error checking/handling, as seen so often...  :-| (Not
Tobias' fault here, of course.)  I'm not going to fix up this one case,
as there are so many other pre-existing ones already...

What I don't understand is why this only did hang occasionally but not
all the time...  (What "junk" could there be on 'stdin' to read, or why
would 'stdin' just sometimes be closed, EOF?)

With that fixed, we now get the expected
'build-gcc/gcc/omp-device-properties-gcn', and:

    $ diff -U1 [...] build-gcc/gcc/omp-device-properties.h
    --- [...] 2020-04-23 10:03:38.278178193 +0200
    +++ build-gcc/gcc/omp-device-properties.h       2020-04-23 22:16:52.472436595 +0200
    @@ -2,2 +2,3 @@
     "amdgcn-amdhsa\0"
    +"gpu\0\0"
     "nvptx-none\0"
    @@ -9,2 +10,3 @@
     "amdgcn-amdhsa\0"
    +"gcn\0\0"
     "nvptx-none\0"
    @@ -16,2 +18,3 @@
     "amdgcn-amdhsa\0"
    +"fiji\0gfx900\0gfx906\0\0"
     "nvptx-none\0"


Doesn't this thing need some testsuite coverage, like
'c-c++-common/gomp/declare-variant-9.c',
'c-c++-common/gomp/declare-variant-10.c'?  (... which would've caught the
problem I've run into here?)


Also, nvptx testsuite coverage could be better, too...  :-|


Grüße
 Thomas


-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gcn-Fix-omp-device-properties-gcn-handling.patch --]
[-- Type: text/x-diff, Size: 4329 bytes --]

From b6a0ae1d22c9675f4374c2cb2b5c0833bb1461f1 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Thu, 23 Apr 2020 21:45:34 +0200
Subject: [PATCH] [gcn] Fix 'omp-device-properties-gcn' handling

Fix-up for commit 955cd057454b323419e06affa7df7d59dc3cd1fb "Add
gcc/config/gcn/t-omp-device for OpenMP declare variant kind/arch/isa".

With AMD GCN offloading configured, I'm seeing occasional GCC build hangs.
I've now captured and analyzed one of them:

    $ ps -f
    UID        PID  PPID  C STIME TTY          TIME CMD
    [...]
    tschwing  5113  4508  0 20:24 pts/5    00:00:00 /bin/sh -c rm -f tmp-omp-device-properties.h; \ for kind in kind arch isa; do \   echo 'const char omp_offload_device_'${kind}'[] = ' \     >> tmp-omp-device-properties.h; \   for prop in no
    tschwing  5126  5113  0 20:24 pts/5    00:00:00 sed -n s/^kind: //p
    tschwing  5127  5113  0 20:24 pts/5    00:00:00 sed s/[[:blank:]]/ /g;s/  */ /g;s/^ //;s/ $//;s/ /\\0/g;s/^/"/;s/$/\\0\\0"/
    [...]
    $ pstree -p $$
    [...]---sh(5113)-+-sed(5126)
                     `-sed(5127)
    $ ls -lrt build-gcc/gcc/*omp-device*
    -rw-r--r-- 1 tschwing eeg  39 Apr 23 20:24 build-gcc/gcc/omp-device-properties-nvptx
    -rw-r--r-- 1 tschwing eeg 634 Apr 23 20:24 build-gcc/gcc/omp-device-properties-i386
    -rw-r--r-- 1 tschwing eeg  58 Apr 23 20:24 build-gcc/gcc/tmp-omp-device-properties.h

Notably missing is the 'omp-device-properties-gcn' file...

    $ grep ^ build-gcc/gcc/*omp-device*
    build-gcc/gcc/omp-device-properties-i386:kind: cpu
    build-gcc/gcc/omp-device-properties-i386:arch: x86 x86_64 i386 i486 i586 i686 ia32
    build-gcc/gcc/omp-device-properties-i386:isa: sse4 cx16 [...]
    build-gcc/gcc/omp-device-properties-nvptx:kind: gpu
    build-gcc/gcc/omp-device-properties-nvptx:arch: nvptx
    build-gcc/gcc/omp-device-properties-nvptx:isa: sm_30 sm_35
    build-gcc/gcc/tmp-omp-device-properties.h:const char omp_offload_device_kind[] =
    build-gcc/gcc/tmp-omp-device-properties.h:"amdgcn-amdhsa\0"

..., which we here seem to be intending to fill into
'tmp-omp-device-properties.h'.

    $ grep ^omp_device_properties\ = build-gcc/gcc/Makefile
    omp_device_properties =  amdgcn-amdhsa= nvptx-none=omp-device-properties-nvptx x86_64-intelmicemul-linux-gnu=omp-device-properties-i386

Given the 's-omp-device-properties-h' Makefile rule, indeed there is an
unescaped '$${props}', which is meant to be the filename following the equals
sign -- but there is none for 'amdgcn-amdhsa=', so this tries to read from
'stdin'!

The real problem of course is elsewhere.

	gcc/
	* configure.ac <$enable_offload_targets>: 'amdgcn' is 'gcn'.
	* configure: Regenerate.
---
 gcc/ChangeLog    | 3 +++
 gcc/configure    | 2 +-
 gcc/configure.ac | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index ef851ef84626..85d1c2b6f758 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,5 +1,8 @@
 2020-04-29  Thomas Schwinge  <thomas@codesourcery.com>
 
+	* configure.ac <$enable_offload_targets>: 'amdgcn' is 'gcn'.
+	* configure: Regenerate.
+
 	PR target/94279
 	* rtlanal.c (set_noop_p): Handle non-constant selectors.
 
diff --git a/gcc/configure b/gcc/configure
index 9e22c5a286f6..83101072aea0 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -7924,7 +7924,7 @@ for tgt in `echo $enable_offload_targets | sed 's/,/ /g'`; do
 	omp_device_property=omp-device-properties-i386
 	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/i386/t-omp-device"
 	;;
-      gcn*-*)
+      amdgcn*-*)
 	omp_device_property=omp-device-properties-gcn
 	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/gcn/t-omp-device"
 	;;
diff --git a/gcc/configure.ac b/gcc/configure.ac
index cd62312b813c..b604047ae456 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -1049,7 +1049,7 @@ for tgt in `echo $enable_offload_targets | sed 's/,/ /g'`; do
 	omp_device_property=omp-device-properties-i386
 	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/i386/t-omp-device"
 	;;
-      gcn*-*)
+      amdgcn*-*)
 	omp_device_property=omp-device-properties-gcn
 	omp_device_property_tmake_file="${omp_device_property_tmake_file} \$(srcdir)/config/gcn/t-omp-device"
 	;;
-- 
2.26.2