gcn/t-omp-device: Add 'amdgcn' as 'arch' [PR105602]

  While 'vendor' and 'kind' is well defined, 'arch' and 'isa' isn't.

When looking at an 'metadirective' testcase (which oddly uses 'arch(amd)'),
I noticed that LLVM uses 'arch(amdgcn)' while we use 'gcn', cf. e.g.
'clang/lib/Headers/openmp_wrappers/math.h'.
(Side note: we use the target triplet amdgcn-amdhsa and LLVM uses amdgcn-amd-amdhsa.)

Given the target triplet, the LLVM choice seems to make more sense; 'gcn'
only shows up as directory name (under gcc/config + libgomp/).

Thus, I think we have two options:
* Either also change to (only) 'amdgcn' - given that the supported arch()
   values are neither documented not yet widely used.
* Or add 'amdgcn' alongside 'gcn' - which is what the attached patch does.

Which option do you prefer? Other thoughts?

Tobias

PS: I think during the GCC 13 development, we should start adding more
offload-target documentation, including documenting the used/permitted
vendor/arch/isa context selectors.
-----------------
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
  

On 16/05/2022 11:28, Tobias Burnus wrote:
> While 'vendor' and 'kind' is well defined, 'arch' and 'isa' isn't.
> 
> When looking at an 'metadirective' testcase (which oddly uses 'arch(amd)'),
> I noticed that LLVM uses 'arch(amdgcn)' while we use 'gcn', cf. e.g.
> 'clang/lib/Headers/openmp_wrappers/math.h'.
> (Side note: we use the target triplet amdgcn-amdhsa and LLVM uses 
> amdgcn-amd-amdhsa.)
> 
> Given the target triplet, the LLVM choice seems to make more sense; 'gcn'
> only shows up as directory name (under gcc/config + libgomp/).
> 
> Thus, I think we have two options:
> * Either also change to (only) 'amdgcn' - given that the supported arch()
>    values are neither documented not yet widely used.
> * Or add 'amdgcn' alongside 'gcn' - which is what the attached patch does.
> 
> Which option do you prefer? Other thoughts?

As long as this remains backwards compatible and fixes your problem, 
accepting everything that might occur is fine with me. I don't think 
trying to pick one is realistic; whatever we choose, there'll always be 
some other toolchain doing something different.

Andrew
  

On Mon, May 16, 2022 at 12:28:45PM +0200, Tobias Burnus wrote:
> While 'vendor' and 'kind' is well defined, 'arch' and 'isa' isn't.
> 
> When looking at an 'metadirective' testcase (which oddly uses 'arch(amd)'),
> I noticed that LLVM uses 'arch(amdgcn)' while we use 'gcn', cf. e.g.
> 'clang/lib/Headers/openmp_wrappers/math.h'.
> (Side note: we use the target triplet amdgcn-amdhsa and LLVM uses amdgcn-amd-amdhsa.)
> 
> Given the target triplet, the LLVM choice seems to make more sense; 'gcn'
> only shows up as directory name (under gcc/config + libgomp/).
> 
> Thus, I think we have two options:
> * Either also change to (only) 'amdgcn' - given that the supported arch()
>   values are neither documented not yet widely used.
> * Or add 'amdgcn' alongside 'gcn' - which is what the attached patch does.
> 
> Which option do you prefer? Other thoughts?

I prefer gcn and amdgcn as aliases of each other.

> PS: I think during the GCC 13 development, we should start adding more
> offload-target documentation, including documenting the used/permitted
> vendor/arch/isa context selectors.
> -----------------
> 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/t-omp-device: Add 'amdgcn' as 'arch' [PR105602]
> 
> Improve cross-compiler handling.
> 
> gcc/ChangeLog:
> 
> 	PR target/105602
> 	* config/gcn/t-omp-device (arch): Add 'amdgcn' besides existing 'gcn'.
> 
> diff --git a/gcc/config/gcn/t-omp-device b/gcc/config/gcn/t-omp-device
> index cd56e2f8a68..e1d9e0d2a1e 100644
> --- a/gcc/config/gcn/t-omp-device
> +++ b/gcc/config/gcn/t-omp-device
> @@ -1,4 +1,4 @@
>  omp-device-properties-gcn: $(srcdir)/config/gcn/gcn.cc
>  	echo kind: gpu > $@
> -	echo arch: gcn >> $@
> +	echo arch: amdgcn gcn >> $@
>  	echo isa: fiji gfx900 gfx906 gfx908 >> $@

But the above patch only implements it partially.
What is in omp-device-properties-* is for the sake of the host compiler,
so that it knows possible support by each offloading target.
If arch or isa or kind isn't supported by either host or by any of the
possible configured offloading target, it can be considered not matching
right away (well, right now that is around gimplification time).
Similarly if it doesn't match host and can't be in offloaded code.
Otherwise, the decision is postponed until after IPA, where we are sure if
it is offloaded or host code and for which exact arch/isa/kind it is.

Without your patch, we'll say arch(amdgcn) doesn't match right away.
With your patch, we'll defer it and reject after IPA.

You need to also change gcc/config/gcn/gcn.cc (gcn_omp_device_kind_arch_isa)
case omp_device_arch: handling so that it accepts both "gcn" and "amdgcn"
equally.

	Jakub
  

Hi Jakub, hi Andrew,

On 17.05.22 10:01, Jakub Jelinek wrote:
> But the above patch only implements it partially.
> What is in omp-device-properties-* is for the sake of the host compiler,
> [...]
> You need to also change gcc/config/gcn/gcn.cc (gcn_omp_device_kind_arch_isa)
> case omp_device_arch: handling so that it accepts both "gcn" and "amdgcn"
> equally.

Done with the attached patch, which I intent to commit after the lunch break,
unless there are further comments.

* * *

Additionally, I am considering to document the permitted values → second patch.
The idea is to later add more device-specific information, separately for gcn
and nvptx like simd/teams/threads handling or similar information (additional
envvar, where it makes sense etc.), which is currently spread over several
slides, wikipages, mouth-to-mouth information etc.

Thoughts regarding the second patch?

Tobias
-----------------
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
  

On Tue, May 17, 2022 at 02:45:09PM +0200, Tobias Burnus wrote:
> Hi Jakub, hi Andrew,
> 
> On 17.05.22 10:01, Jakub Jelinek wrote:
> > But the above patch only implements it partially.
> > What is in omp-device-properties-* is for the sake of the host compiler,
> > [...]
> > You need to also change gcc/config/gcn/gcn.cc (gcn_omp_device_kind_arch_isa)
> > case omp_device_arch: handling so that it accepts both "gcn" and "amdgcn"
> > equally.
> 
> Done with the attached patch, which I intent to commit after the lunch break,
> unless there are further comments.
> 
> * * *
> 
> Additionally, I am considering to document the permitted values → second patch.
> The idea is to later add more device-specific information, separately for gcn
> and nvptx like simd/teams/threads handling or similar information (additional
> envvar, where it makes sense etc.), which is currently spread over several
> slides, wikipages, mouth-to-mouth information etc.
> 
> Thoughts regarding the second patch?
> 
> Tobias
> -----------------
> 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/t-omp-device: Add 'amdgcn' as 'arch' [PR105602]
> 
> Improve cross-compiler handling.
> 
> gcc/ChangeLog:
> 
> 	PR target/105602
> 	* config/gcn/t-omp-device (arch): Add 'amdgcn' besides existing 'gcn'.
> 	* config/gcn/gcn.cc (gcn_omp_device_kind_arch_isa): Likewise.
> 
>  gcc/config/gcn/gcn.cc       | 2 +-
>  gcc/config/gcn/t-omp-device | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/config/gcn/gcn.cc b/gcc/config/gcn/gcn.cc
> index e2e9335ad75..92896ab972f 100644
> --- a/gcc/config/gcn/gcn.cc
> +++ b/gcc/config/gcn/gcn.cc
> @@ -2632,7 +2632,7 @@ gcn_omp_device_kind_arch_isa (enum omp_device_kind_arch_isa trait,
>      case omp_device_kind:
>        return strcmp (name, "gpu") == 0;
>      case omp_device_arch:
> -      return strcmp (name, "gcn") == 0;
> +      return (strcmp (name, "amdgcn") == 0 || strcmp (name, "gcn") == 0);

The ()s around it aren't needed and don't make it more readable.

Otherwise LGTM.

> +@headitem @code{arch} @tab @code{kind} @tab @code{isa}
> +@item @code{intel_mic}, @code{86}, @code{86_64}, @code{386}, @code{486},
> +      @code{586}, @code{686}, @code{a32}

You've lost the first letter of most of the above arches.
x86, x86_64, i386, i486, i586, i686, ia32

Otherwise LGTM.

	Jakub