[gcn] Add -mgpu option and plumb in assembler/linker

This patch, for the "gcn" branch, does three things:

1. Add specs to drive the LLVM assembler and linker. It requires them to 
be installed as "as" and "ld", under $target/bin, but then the compiler 
Just Works with these specs.

2. Switch to HSACO format version 2, and have the assembler auto-set the 
architecture flags from -mcpu. This means the amdphdr utility is no 
longer required.

3. Add -mgpu option and corresponding --with-gpu. I've deliberately used 
"gpu" instead of "cpu" because I want offloading compilers to be able to 
say "-mcpu=foo -foffload=-mgpu=bar", or even have the host compiler just 
understand -mgpu and DTRT.

The patch also removes the unused and unwritten "arch" and "tune" 
settings. They can be added back when useful, but the assembler requires 
a GPU name, I think, so we need that as input.

OK to commit to GCN branch?

Andrew

On Fri, 28 Apr 2017, Andrew Stubbs wrote:

> 3. Add -mgpu option and corresponding --with-gpu. I've deliberately used "gpu"
> instead of "cpu" because I want offloading compilers to be able to say
> "-mcpu=foo -foffload=-mgpu=bar", or even have the host compiler just
> understand -mgpu and DTRT.

Command-line options should be documented in invoke.texi, configure 
options in install.texi.

Hi Andrew,

sorry for replying only now but yesterday was public holiday here and
I am still only in the process of recovering from a long weekend.

While the only objection I have is the C++ style comment in
config/gcn/gcn.c, another problem, for me at least...

On Fri, Apr 28, 2017 at 06:06:39PM +0100, Andrew Stubbs wrote:
> This patch, for the "gcn" branch, does three things:
> 
> 1. Add specs to drive the LLVM assembler and linker. It requires them to be
> installed as "as" and "ld", under $target/bin, but then the compiler Just
> Works with these specs.

...is that I do not have llvm linker at hand and without it I did not
manage to make the patch produce loadable code.  Because ROCm 1.5 has
been released today, I will update our environment, which is a bit
obsolete, get llvm ld and try again.  This might take me a few days,
so please bear with me for a little more, I would like to make sure it
works on carrizos.

Thanks,

Martin

> 
> 2. Switch to HSACO format version 2, and have the assembler auto-set the
> architecture flags from -mcpu. This means the amdphdr utility is no longer
> required.
> 
> 3. Add -mgpu option and corresponding --with-gpu. I've deliberately used
> "gpu" instead of "cpu" because I want offloading compilers to be able to say
> "-mcpu=foo -foffload=-mgpu=bar", or even have the host compiler just
> understand -mgpu and DTRT.
> 
> The patch also removes the unused and unwritten "arch" and "tune" settings.
> They can be added back when useful, but the assembler requires a GPU name, I
> think, so we need that as input.
> 
> OK to commit to GCN branch?
>     2017-04-28  Andrew Stubbs  <ams@codesourcery.com>
>     
>     	gcc/
>     	* config.gcc (amdgcn): Remove --with-arch and --with-tune.
>     	Add --with-gpu, and set default to "carrizo"
>     	(add_defaults): Add "gpu".
>     	* config/gcn/gcn-opts.h: New file.
>     	* config/gcn/gcn.c (output_file_start): Switch to HSACO version
>     	2 and auto-detection of GPU type (from -mcpu).
>     	(gcn_arch, gcn_tune): Remove.
>     	* config/gcn/gcn.h: Include gcn-opts.h.
>     	(enum processor_type): Move to gcn-opts.h.
>     	(LIBGCC_SPEC, ASM_SPEC, LINK_SPEC): Define.
>     	(gcn_arch, gcn_tune): Remove.
>     	(OPTION_DEFAULT_SPECS): Remove "arch" and "tune"; add "gpu".
>     	* config/gcn/gcn.opt: Include gcn-opts.h.
>     	(gpu_type): New Enum.
>     	(mgpu): New option.
>

On 02/05/17 18:08, Martin Jambor wrote:
> Hi Andrew,
> 
> sorry for replying only now but yesterday was public holiday here and
> I am still only in the process of recovering from a long weekend.

No problem, the UK had the same. :-)

> While the only objection I have is the C++ style comment in
> config/gcn/gcn.c, another problem, for me at least...
> 
> On Fri, Apr 28, 2017 at 06:06:39PM +0100, Andrew Stubbs wrote:
>> This patch, for the "gcn" branch, does three things:
>>
>> 1. Add specs to drive the LLVM assembler and linker. It requires them to be
>> installed as "as" and "ld", under $target/bin, but then the compiler Just
>> Works with these specs.
> 
> ...is that I do not have llvm linker at hand and without it I did not
> manage to make the patch produce loadable code.  Because ROCm 1.5 has
> been released today, I will update our environment, which is a bit
> obsolete, get llvm ld and try again.  This might take me a few days,
> so please bear with me for a little more, I would like to make sure it
> works on carrizos.

Understood. I do not have a carrizo to test on.

All my testing will be with FuryX Fiji discrete GPUs. The main 
difference, from the software point of view, is that the shared memory 
must be handled a little differently. (The existing HSA back-end appears 
incompatible, as are the samples from the HSA Foundation sources; the 
ROCm samples work fine.) However, the HSACO binary must encode the right 
magic numbers or the driver rejects it, hence the need for this patch.

Andrew

On 02/05/17 18:08, Martin Jambor wrote:
>> 1. Add specs to drive the LLVM assembler and linker. It requires them to be
>> installed as "as" and "ld", under $target/bin, but then the compiler Just
>> Works with these specs.
> 
> ...is that I do not have llvm linker at hand and without it I did not
> manage to make the patch produce loadable code.  Because ROCm 1.5 has
> been released today, I will update our environment, which is a bit
> obsolete, get llvm ld and try again.  This might take me a few days,
> so please bear with me for a little more, I would like to make sure it
> works on carrizos.

Any news?

If you're happy with the patch I can fix the comment style, and add the 
documentation Joseph asked for, and get it committed.

Andrew

Hello Andrew,

I apologize for taking so long to reply, I was traveling for two past
weeks and just before that we suffered some local infrastructure
issues that prevented me from working on this too.

On Fri, Apr 28, 2017 at 06:06:39PM +0100, Andrew Stubbs wrote:
> This patch, for the "gcn" branch, does three things:
> 
> 1. Add specs to drive the LLVM assembler and linker. It requires them to be
> installed as "as" and "ld", under $target/bin, but then the compiler Just
> Works with these specs.

At the moment I prefer to use --with-as and --with-ld configure
options which are better suited for my setup.  The invocation of
assembler works well, the invocation of ld.lld works too, but with the
added caveat that collect2 afterwards attempts to do non-plufin LTO
and calls maybe_run_lto_and_relink, which wants to run nm, which is
not available and so it fails with a fatal_error.  It took me a while
to figure out what was going on and that the result was actually fine,
despite the error message.  I guess we are fine with passing -fno-lto
or rather disabling lto at configure time for the time being.

> 
> 2. Switch to HSACO format version 2, and have the assembler auto-set the
> architecture flags from -mcpu. This means the amdphdr utility is no longer
> required.

This is the one thing that was it difficult for me to get it working.
I had to upgrade my kernel and both run-time libraries to the newest
ROCm 1.5, re-compile llvm and lld from ROCm github branches, and
rewrite our testing kernel invoker to use non-deprecated HSA 1.1
functions (we had been using hsa_code_object_deserialize and friends
from HSA 1.0).  But finally, my kernels get loaded, started and work.

> 
> 3. Add -mgpu option and corresponding --with-gpu. I've deliberately used
> "gpu" instead of "cpu" because I want offloading compilers to be able to say
> "-mcpu=foo -foffload=-mgpu=bar", or even have the host compiler just
> understand -mgpu and DTRT.

As far as I am concerned, this seems like a good idea. 

Anyhow, thanks for submitting your patch, I apologize once again for
taking so long to test it.  Please commit the changes, I will wait
with (a bit overdue) merge from trunk until after you do.

Thanks,

Martin


> 
> The patch also removes the unused and unwritten "arch" and "tune" settings.
> They can be added back when useful, but the assembler requires a GPU name, I
> think, so we need that as input.
> 
> OK to commit to GCN branch?
> 
> Andrew
> 

> commit 5058457b0fa07865b366832828e74a53e5bd2964
> Author: Andrew Stubbs <ams@codesourcery.com>
> Date:   Fri Apr 28 14:37:25 2017 +0100
> 
>     Add -mgpu
>     
>     2017-04-28  Andrew Stubbs  <ams@codesourcery.com>
>     
>     	gcc/
>     	* config.gcc (amdgcn): Remove --with-arch and --with-tune.
>     	Add --with-gpu, and set default to "carrizo"
>     	(add_defaults): Add "gpu".
>     	* config/gcn/gcn-opts.h: New file.
>     	* config/gcn/gcn.c (output_file_start): Switch to HSACO version
>     	2 and auto-detection of GPU type (from -mcpu).
>     	(gcn_arch, gcn_tune): Remove.
>     	* config/gcn/gcn.h: Include gcn-opts.h.
>     	(enum processor_type): Move to gcn-opts.h.
>     	(LIBGCC_SPEC, ASM_SPEC, LINK_SPEC): Define.
>     	(gcn_arch, gcn_tune): Remove.
>     	(OPTION_DEFAULT_SPECS): Remove "arch" and "tune"; add "gpu".
>     	* config/gcn/gcn.opt: Include gcn-opts.h.
>     	(gpu_type): New Enum.
>     	(mgpu): New option.
>

Hi!

Sorry for the late reply.

On Fri, 28 Apr 2017 18:06:39 +0100, Andrew Stubbs <ams@codesourcery.com> wrote:
> 3. Add -mgpu option and corresponding --with-gpu. I've deliberately used 
> "gpu" instead of "cpu" because I want offloading compilers to be able to 
> say "-mcpu=foo -foffload=-mgpu=bar", or even have the host compiler just 
> understand -mgpu and DTRT.

I'm not sure I understand your last statement, or the intentions behind
it.

How would the host compiler (be able to) understand (or, disambiguate)
"-mgpu=[...]" in the (default) case of several offloading targets having
been configured?  I think it holds that "-m[...]" etc. must/can always
only apply to the current target (or "host", in "offloading speak").

And then, I don't have any strong opinion, but I don't see why a new
"-mgpu" option is preferable to using the existing "-march" etc. in
"-foffload=[...]".  For example, you can already now do things like
(exemplary):

    -march=x86_64 -foffload=-march=generic -foffload=nvptx-none=-march=cc_50 -foffload=gcn=-march=carrizo
    ^ target-specific ^ offload-target-specific, unless overridden... ^ ... here... ^ ..., and here

Likewise for the new "--with-gpu=[...]" vs. the existing
"--with-arch=[...]", where again I would, unless there is a specific
reason (that I didn't understand here), default to using the existing
option names instead of introducing new ones.


Grüße
 Thomas


> commit 5058457b0fa07865b366832828e74a53e5bd2964
> Author: Andrew Stubbs <ams@codesourcery.com>
> Date:   Fri Apr 28 14:37:25 2017 +0100
> 
>     Add -mgpu
>     
>     2017-04-28  Andrew Stubbs  <ams@codesourcery.com>
>     
>     	gcc/
>     	* config.gcc (amdgcn): Remove --with-arch and --with-tune.
>     	Add --with-gpu, and set default to "carrizo"
>     	(add_defaults): Add "gpu".
>     	* config/gcn/gcn-opts.h: New file.
>     	* config/gcn/gcn.c (output_file_start): Switch to HSACO version
>     	2 and auto-detection of GPU type (from -mcpu).
>     	(gcn_arch, gcn_tune): Remove.
>     	* config/gcn/gcn.h: Include gcn-opts.h.
>     	(enum processor_type): Move to gcn-opts.h.
>     	(LIBGCC_SPEC, ASM_SPEC, LINK_SPEC): Define.
>     	(gcn_arch, gcn_tune): Remove.
>     	(OPTION_DEFAULT_SPECS): Remove "arch" and "tune"; add "gpu".
>     	* config/gcn/gcn.opt: Include gcn-opts.h.
>     	(gpu_type): New Enum.
>     	(mgpu): New option.
> 
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index 4a77b66..b1df533 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -3901,20 +3901,20 @@ case "${target}" in
>  		;;
>  
>  	amdgcn-*-*)
> -		supported_defaults="arch tune"
> +		supported_defaults="gpu"
>  
> -		for which in arch tune; do
> -			eval "val=\$with_$which"
> -			case ${val} in
> -			"" | fiji)
> -				# OK
> -				;;
> -			*)
> -				echo "Unknown cpu used in --with-$which=$val." 1>&2
> -				exit 1
> -				;;
> -			esac
> -		done
> +		case "$with_gpu" in
> +		"")
> +			with_gpu=carrizo
> +			;;
> +		carrizo | fiji)
> +			# OK
> +			;;
> +		*)
> +			echo "Unknown gpu used in --with-gpu=$val." 1>&2
> +			exit 1
> +			;;
> +		esac
>  		;;
>  
>  	hppa*-*-*)
> @@ -4646,7 +4646,7 @@ case ${target} in
>  esac
>  
>  t=
> -all_defaults="abi cpu cpu_32 cpu_64 arch arch_32 arch_64 tune tune_32 tune_64 schedule float mode fpu nan fp_32 odd_spreg_32 divide llsc mips-plt synci tls lxc1-sxc1 madd4"
> +all_defaults="abi cpu cpu_32 cpu_64 arch arch_32 arch_64 tune tune_32 tune_64 schedule float mode fpu nan fp_32 odd_spreg_32 divide llsc mips-plt synci tls lxc1-sxc1 madd4 gpu"
>  for option in $all_defaults
>  do
>  	eval "val=\$with_"`echo $option | sed s/-/_/g`
> diff --git a/gcc/config/gcn/gcn-opts.h b/gcc/config/gcn/gcn-opts.h
> new file mode 100644
> index 0000000..d0586d6
> --- /dev/null
> +++ b/gcc/config/gcn/gcn-opts.h
> @@ -0,0 +1,27 @@
> +/* Copyright (C) 2016-2017 Free Software Foundation, Inc.
> +
> +   This file is free software; you can redistribute it and/or modify it under
> +   the terms of the GNU General Public License as published by the Free
> +   Software Foundation; either version 3 of the License, or (at your option)
> +   any later version.
> +
> +   This file is distributed in the hope that it will be useful, but WITHOUT
> +   ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +   FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> +   for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with GCC; see the file COPYING3.  If not see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GCN_OPTS_H
> +#define GCN_OPTS_H
> +
> +/* Which processor to generate code or schedule for.  */
> +enum processor_type
> +{
> +  PROCESSOR_CARRIZO,
> +  PROCESSOR_FIJI
> +};
> +
> +#endif
> diff --git a/gcc/config/gcn/gcn.c b/gcc/config/gcn/gcn.c
> index eb6edd8..f378bf8 100644
> --- a/gcc/config/gcn/gcn.c
> +++ b/gcc/config/gcn/gcn.c
> @@ -60,11 +60,6 @@
>  /* This file should be included last.  */
>  #include "target-def.h"
>  
> -/* Which instruction set architecture to use.  */
> -int gcn_arch;
> -/* Which cpu are we tuning for.  */
> -int gcn_tune;
> -
>  static REAL_VALUE_TYPE dconst4, dconst1over2pi;
>  static bool ext_gcn_constants_init = 0;
>  
> @@ -2006,8 +2001,8 @@ static void
>  output_file_start (void)
>  {
>    fprintf (asm_out_file, "\t.hsatext\n");
> -  fprintf (asm_out_file, "\t.hsa_code_object_version 1,0\n");
> -  fprintf (asm_out_file, "\t.hsa_code_object_isa 8,0,1,\"AMD\",\"AMDGPU\"\n");
> +  fprintf (asm_out_file, "\t.hsa_code_object_version 2,0\n");
> +  fprintf (asm_out_file, "\t.hsa_code_object_isa\n");  // Autodetect
>    fprintf (asm_out_file, "\t.section\t.AMDGPU.config\n");
>    fprintf (asm_out_file, "\t.hsatext\n");
>  }
> diff --git a/gcc/config/gcn/gcn.h b/gcc/config/gcn/gcn.h
> index 903022f..a3f9463 100644
> --- a/gcc/config/gcn/gcn.h
> +++ b/gcc/config/gcn/gcn.h
> @@ -14,25 +14,28 @@
>     along with GCC; see the file COPYING3.  If not see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include "config/gcn/gcn-opts.h"
> +
>  
>  /* FIXME */
>  #define TARGET_CPU_CPP_BUILTINS()
>  
> -/* Which processor to generate code or schedule for.  */
> -enum processor_type
> -{
> -  PROCESSOR_CARRIZO,
> -};
> +/* Temporarily disable libgcc until one actually exists.  */
> +#undef  LIBGCC_SPEC
> +#define LIBGCC_SPEC ""
> +
> +/* Use LLVM assembler options.  */
> +#undef ASM_SPEC
> +#define ASM_SPEC "-triple=amdgcn--amdhsa %{mgpu=*:-mcpu=%*} -filetype=obj"
>  
> -extern GTY(()) int gcn_arch;
> -extern GTY(()) int gcn_tune;
> +/* Default to relocatable executables as output.  */
> +#undef LINK_SPEC
> +#define LINK_SPEC "-shared"
>  
>  /* Support for a compile-time default architecture and tuning.  The rules are:
> -   --with-arch is ignored if -march is specified.
> -   --with-tune is ignored if -mtune is specified.  */
> +   --with-gpu is ignored if -mgpu is specified.  */
>  #define OPTION_DEFAULT_SPECS \
> -  {"arch", "%{!march=*:-march=%(VALUE)}" }, \
> -  {"tune", "%{!mtune=*:-mtune=%(VALUE)}" }
> +  {"gpu", "%{!mgpu=*:-mgpu=%(VALUE)}"}
>  
>  /* Default target_flags if no switches specified.  */
>  #ifndef TARGET_DEFAULT
> diff --git a/gcc/config/gcn/gcn.opt b/gcc/config/gcn/gcn.opt
> index 8fc02b7..77f0ef0 100644
> --- a/gcc/config/gcn/gcn.opt
> +++ b/gcc/config/gcn/gcn.opt
> @@ -17,3 +17,20 @@
>  ; You should have received a copy of the GNU General Public License
>  ; along with GCC; see the file COPYING3.  If not see
>  ; <http://www.gnu.org/licenses/>.
> +
> +HeaderInclude
> +config/gcn/gcn-opts.h
> +
> +Enum
> +Name(gpu_type) Type(enum processor_type)
> +GCN GPU type to use:
> +
> +EnumValue
> +Enum(gpu_type) String(carrizo) Value(PROCESSOR_CARRIZO)
> +
> +EnumValue
> +Enum(gpu_type) String(fiji) Value(PROCESSOR_FIJI)
> +
> +mgpu=
> +Target RejectNegative Joined ToLower Enum(gpu_type) Var(gcn_gpu) Init(PROCESSOR_CARRIZO)
> +Specify the name of the target GPU.

On 29/05/17 18:27, Martin Jambor wrote:
> I apologize for taking so long to reply, I was traveling for two past
> weeks and just before that we suffered some local infrastructure
> issues that prevented me from working on this too.

And I've just been on vacation for a week. :-)

> On Fri, Apr 28, 2017 at 06:06:39PM +0100, Andrew Stubbs wrote:
> At the moment I prefer to use --with-as and --with-ld configure

Those only work with absolute paths, which is not suitable for a 
toolchain that may be installed in a user's home directory, which will 
be my use case.

> despite the error message.  I guess we are fine with passing -fno-lto
> or rather disabling lto at configure time for the time being.

I configure with --disable-lto.

> This is the one thing that was it difficult for me to get it working.
> I had to upgrade my kernel and both run-time libraries to the newest
> ROCm 1.5, re-compile llvm and lld from ROCm github branches, and
> rewrite our testing kernel invoker to use non-deprecated HSA 1.1
> functions (we had been using hsa_code_object_deserialize and friends
> from HSA 1.0).  But finally, my kernels get loaded, started and work.

Hmm, I didn't realize it would be so hard. I guess I'm joining the party 
late.

>> 3. Add -mgpu option and corresponding --with-gpu. I've deliberately used
>> "gpu" instead of "cpu" because I want offloading compilers to be able to say
>> "-mcpu=foo -foffload=-mgpu=bar", or even have the host compiler just
>> understand -mgpu and DTRT.
> 
> As far as I am concerned, this seems like a good idea.

Thomas objects to the new option, and after talking with him the 
reasoning seems sound. GCC has been moving away from -mcpu in any case, 
so I guess I'll put -march and -mtune back, and use those for the same 
purpose.

I'll commit the patch with those changes soonish.

Andrew