Hi Prathamesh!
On 2024-08-12T07:50:07+0000, Prathamesh Kulkarni <prathame...@nvidia.com> wrote:
>> From: Thomas Schwinge <tschwi...@baylibre.com>
>> Sent: Friday, August 9, 2024 12:55 AM
>> On 2024-08-08T06:46:25-0700, Andrew Pinski <pins...@gmail.com> wrote:
>> > On Thu, Aug 8, 2024 at 6:11 AM Prathamesh Kulkarni
>> > <prathame...@nvidia.com> wrote:
>> >> After differing NUM_POLY_INT_COEFFS fix for AArch64/nvptx
>> offloading, the following minimal test:
>>
>> First, thanks for your work on enabling this! I will say that I had
>> the plan to re-engage with Nvidia to hire us (as initial implementors
>> of GCC/nvptx offloading) to make AArch64/nvptx offloading work, but
>> now that Nvidia has its own GCC team, that's great that you're able to
>> work on this yourself! :-)
>>
>> Please CC me for GCC/nvptx issues for (at least potentially...) faster
>> response times.
> Thanks, will do 😊
Heh, so much for "potentially": I'm not able to spend a lot of time on
this right now, as I shall soon be out of office. Quickly:
>> >> compiled with -fopenmp -foffload=nvptx-none now fails with:
>> >> gcc: error: unrecognized command-line option '-m64'
>> >> nvptx mkoffload: fatal error: ../install/bin/gcc returned 1 exit
>> status compilation terminated.
>>
>> Heh. Yeah...
>>
>> >> As mentioned in RFC email, this happens because
>> >> nvptx/mkoffload.cc:compile_native passes -m64/-m32 to host compiler
>> depending on whether offload_abi is OFFLOAD_ABI_LP64 or
>> OFFLOAD_ABI_ILP32, and aarch64 backend doesn't recognize these
>> options.
>> So, my idea is: instead of the current strategy that the host
>> 'TARGET_OFFLOAD_OPTIONS' synthesizes '-foffload-abi=lp64' etc., which
>> the 'mkoffload's then interpret and re-synthesize '-m64' etc. -- how
>> about we instead directly tell the 'mkoffload's the relevant ABI
>> options? That is, 'TARGET_OFFLOAD_OPTIONS' instead synthesizes '-
>> foffload-abi=-m64'
>> etc., which the 'mkoffload's can then readily use. Could you please
>> give that a try, and/or does anyone see any issues with that approach?
>>
>> And use something like '-foffload-abi=disable' to replace the current:
>>
>> /* PR libgomp/65099: Currently, we only support offloading in 64-
>> bit
>> configurations. */
>> if (offload_abi == OFFLOAD_ABI_LP64)
>> {
>>
>> (As discussed before, this should be done differently altogether, but
>> that's for another day.)
> Sorry, I don't quite follow. Currently we enable offloading if offload_abi ==
> OFFLOAD_ABI_LP64,
> which is synthesized from -foffload-abi=lp64. If we change -foffload-abi to
> instead specify
> host-specific ABI opts, I guess mkoffload will still need to somehow figure
> out which ABI is used,
> so it can disable offloading for 32-bit ? I suppose we could adjust
> TARGET_OFFLOAD_OPTIONS for each
> host to pass -foffload-abi=disable if TARGET_ILP32 is set and offload target
> is nvptx, but not sure
> if that'd be correct ?
Basically, yes. My idea was that all 'TARGET_OFFLOAD_OPTIONS'
implementations return either the correct host flags to be used by the
'mkoffload's (the case that offloading is supported for the current host
flags/ABI configuration), or otherwise return '-foffload-abi=disable'.
For example (untested):
> char *
> ix86_offload_options (void)
> {
> if (TARGET_LP64)
> - return xstrdup ("-foffload-abi=lp64");
> + return xstrdup ("-foffload-abi=-m64");
> - return xstrdup ("-foffload-abi=ilp32");
> + return xstrdup ("-foffload-abi=disable");
> }
That is, only for 'TARGET_LP64' offloading is supported, and via
'-foffload-abi=-m64' the 'mkoffload's know that they need to specify
'-m64'. For other host flags/ABI configuration, the 'mkoffload's see
'-foffload-abi=disable' and thus disable offload code generation
(replacing the current 'if (offload_abi == OFFLOAD_ABI_LP64)' in
'mkoffload').
> In the attached patch
Yes, that's going in the right direction, thanks!
> I added another option -foffload-abi-host-opts to specify host abi
> opts, and leave -foffload-abi to specify if ABI is 32/64 bit which mkoffload
> can use to
> enable/disable offloading (as before).
I'm not sure however, if this additional option is really necessary?
In case we're not happy to re-purpose the flag name
'-foffload-abi=[...]', we could also rename that one to
'-foffload-abi-host-opts=[...]'; the former is not user-exposed, so we
may change it as necessary. (Or, in other words, go with your proposed
'-foffload-abi-host-opts=[...]', but also remove '-foffload-abi=[...]' at
the same time.)
I'll be able to spend more time on this in two weeks.
Grüße
Thomas
> [nvptx] Pass host specific ABI opts from mkoffload.
>
> The patch adds an option -foffload-abi-host-opts, which
> is set by host in TARGET_OFFLOAD_OPTIONS, and mkoffload then passes it's value
> to host_compiler.
>
> gcc/ChangeLog:
> * common.opt (foffload-abi-host-opts): New option.
> * config/aarch64/aarch64.cc (aarch64_offload_options): Set
> -foffload-abi-host-opts.
> * config/i386/i386-opts.cc (ix86_offload_options): Likewise.
> * config/rs6000/rs6000.cc (rs6000_offload_options): Likewise.
> * config/nvptx/mkoffload.cc (host_abi_opts): Define.
> (compile_native): Append host_abi_opts to argv_obstack.
> (main): Handle option -foffload-abi-host-opts.
> * lto-wrapper.cc (append_compiler_options): Handle
> -foffload-abi-host-opts.
> * opts.cc (common_handle_option): Likewise.
>
> Signed-off-by: Prathamesh Kulkarni <prathame...@nvidia.com>
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index ea39f87ae71..d1a9efb9513 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2361,6 +2361,10 @@ Enum(offload_abi) String(ilp32)
> Value(OFFLOAD_ABI_ILP32)
> EnumValue
> Enum(offload_abi) String(lp64) Value(OFFLOAD_ABI_LP64)
>
> +foffload-abi-host-opts=
> +Common Driver Joined MissingArgError(option or option=abi missing after %qs)
> +-foffload-abi-host-opts=<options>=<abi> Specify host abi options.
> +
> fomit-frame-pointer
> Common Var(flag_omit_frame_pointer) Optimization
> When possible do not generate stack frames.
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 2ac5a22c848..7418cb1fb69 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -18999,9 +18999,9 @@ static char *
> aarch64_offload_options (void)
> {
> if (TARGET_ILP32)
> - return xstrdup ("-foffload-abi=ilp32");
> + return xstrdup ("-foffload-abi=ilp32
> -foffload-abi-host-opts=-mabi=ilp32");
> else
> - return xstrdup ("-foffload-abi=lp64");
> + return xstrdup ("-foffload-abi=lp64 -foffload-abi-host-opts=-mabi=lp64");
> }
>
> static struct machine_function *
> diff --git a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc
> index 1c8f7835af2..bd960674e5d 100644
> --- a/gcc/config/i386/i386-options.cc
> +++ b/gcc/config/i386/i386-options.cc
> @@ -3669,8 +3669,8 @@ char *
> ix86_offload_options (void)
> {
> if (TARGET_LP64)
> - return xstrdup ("-foffload-abi=lp64");
> - return xstrdup ("-foffload-abi=ilp32");
> + return xstrdup ("-foffload-abi=lp64 -foffload-abi-host-opts=-m64");
> + return xstrdup ("-foffload-abi=ilp32 -foffload-abi-host-opts=-m32");
> }
>
> /* Handle "cdecl", "stdcall", "fastcall", "regparm", "thiscall",
> diff --git a/gcc/config/nvptx/mkoffload.cc b/gcc/config/nvptx/mkoffload.cc
> index 503b1abcefd..d5ca2386641 100644
> --- a/gcc/config/nvptx/mkoffload.cc
> +++ b/gcc/config/nvptx/mkoffload.cc
> @@ -61,6 +61,7 @@ static const char *omp_requires_file;
> static const char *ptx_dumpbase;
>
> enum offload_abi offload_abi = OFFLOAD_ABI_UNSET;
> +const char *host_abi_opts = NULL;
>
> /* Delete tempfiles. */
>
> @@ -607,17 +608,9 @@ compile_native (const char *infile, const char *outfile,
> const char *compiler,
> obstack_ptr_grow (&argv_obstack, ptx_dumpbase);
> obstack_ptr_grow (&argv_obstack, "-dumpbase-ext");
> obstack_ptr_grow (&argv_obstack, ".c");
> - switch (offload_abi)
> - {
> - case OFFLOAD_ABI_LP64:
> - obstack_ptr_grow (&argv_obstack, "-m64");
> - break;
> - case OFFLOAD_ABI_ILP32:
> - obstack_ptr_grow (&argv_obstack, "-m32");
> - break;
> - default:
> - gcc_unreachable ();
> - }
> + if (!host_abi_opts)
> + fatal_error (input_location, "-foffload-abi-host-opts not specified.");
> + obstack_ptr_grow (&argv_obstack, host_abi_opts);
> obstack_ptr_grow (&argv_obstack, infile);
> obstack_ptr_grow (&argv_obstack, "-c");
> obstack_ptr_grow (&argv_obstack, "-o");
> @@ -721,6 +714,8 @@ main (int argc, char **argv)
> "unrecognizable argument of option " STR);
> }
> #undef STR
> + else if (startswith (argv[i], "-foffload-abi-host-opts="))
> + host_abi_opts = argv[i] + strlen ("-foffload-abi-host-opts=");
> else if (strcmp (argv[i], "-fopenmp") == 0)
> fopenmp = true;
> else if (strcmp (argv[i], "-fopenacc") == 0)
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 0bcc6a2d0ab..decdf49a1f5 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -17333,9 +17333,9 @@ static char *
> rs6000_offload_options (void)
> {
> if (TARGET_64BIT)
> - return xstrdup ("-foffload-abi=lp64");
> + return xstrdup ("-foffload-abi=lp64 -foffload-abi-host-opts=-m64");
> else
> - return xstrdup ("-foffload-abi=ilp32");
> + return xstrdup ("-foffload-abi=ilp32 -foffload-abi-host-opts=-m32");
> }
>
>
> diff --git a/gcc/lto-wrapper.cc b/gcc/lto-wrapper.cc
> index 6bfc96590a5..1ecc4997e5a 100644
> --- a/gcc/lto-wrapper.cc
> +++ b/gcc/lto-wrapper.cc
> @@ -745,6 +745,7 @@ append_compiler_options (obstack *argv_obstack,
> vec<cl_decoded_option> opts)
> case OPT_fopenacc:
> case OPT_fopenacc_dim_:
> case OPT_foffload_abi_:
> + case OPT_foffload_abi_host_opts_:
> case OPT_fcf_protection_:
> case OPT_fasynchronous_unwind_tables:
> case OPT_funwind_tables:
> diff --git a/gcc/opts.cc b/gcc/opts.cc
> index 0b7b137c376..79118237ce4 100644
> --- a/gcc/opts.cc
> +++ b/gcc/opts.cc
> @@ -3069,6 +3069,7 @@ common_handle_option (struct gcc_options *opts,
> break;
>
> case OPT_foffload_abi_:
> + case OPT_foffload_abi_host_opts_:
> #ifdef ACCEL_COMPILER
> /* Handled in the 'mkoffload's. */
> #else
