Hi!
On 2024-08-16T15:36:29+0000, Prathamesh Kulkarni <prathame...@nvidia.com> wrote:
>> > Am 13.08.2024 um 17:48 schrieb Thomas Schwinge
>> <tschwi...@baylibre.com>:
>> > 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:
>> >>>>> 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'.
Oh..., you're right of course: we do need to continue to tell the
'mkoffload's which kind of offload code to generate! My bad...
>> >> 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?
> Well, my concern was if that'd change the behavior for TARGET_ILP32 ?
> IIUC, currently for -foffload-abi=ilp32, mkoffload will create empty C file
> for ptx_cfile_name (instead of munged ptx assembly since offloading will be
> disabled),
> and pass that to host compiler with -m32 option (in compile_native).
>
> If we change -foffload-abi to specify ABI host opts, and pass
> -foffload-abi=disable
> for TARGET_ILP32 in TARGET_OFFLOAD_OPTIONS, mkoffload will no longer be able
> to
> pass 32-bit ABI opts to host compiler, which may result in linker error (arch
> mismatch?)
> if the host object files are 32-bit ABI and xnvptx-none.o is 64-bit (assuming
> the host
> compiler is configured to generate 64-bit code-gen by default) ?
>
> So, I thought to add another option -foffload-abi-host-opts to pass
> host-specific ABI opts,
> and keep -foffload-abi as-is to infer ABI type for enabling/disabling
> offloading.
Quite right, yes.
>> -----Original Message-----
>> From: Richard Biener <rguent...@suse.de>
>> Sent: Tuesday, August 13, 2024 10:06 PM
>> Since we do not support 32 -> 64 bit offload
We don't -- but it's generally possible. As Tobias recently educated
me, the OpenMP specification explicitly does *not* require matching
host 'sizeof (void *)' and device 'sizeof (void *)'.
At the LLVM workshop at ISC High Performance 2024 there was a (short)
presentation of someone who did LLVM offloading from host to a different
architecture, and from there again to a yet different architecture. Heh!
Anyway:
>> wouldn’t the most
>> pragmatic fix be to recognize -m64 in the nvptx backend (and ignore
>> it)?
> I think nvptx already supports m64 and ignores it.
> From nvptx.opt:
>
> m64
> Target RejectNegative Mask(ABI64)
> Ignored, but preserved for backward compatibility. Only 64-bit ABI is
> supported.
Right, but that's also not the problem here: the problem is that
'mkoffload' puts '-m64' onto the *host* compiler command line (for
embedding the offload image), which in case of aarch64 isn't the right
thing to do; just happened to do the right thing for x86_64 and
powerpc64le.
Prathamesh's proposed patch:
> [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.
ACK, conceptually.
> --- 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.
> +
Here, 'option or option=abi' and '<options>=<abi>' should be just
'options' and '<options>', right? And, TAB between
'-foffload-abi-host-opts=<options>' and its help text. And upper-case
ABI.
> --- 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");
> }
As none of the current offload compilers is set up of ILP32, I suggest we
continue to pass '-foffload-abi=ilp32' without
'-foffload-abi-host-opts=[...]' -- the 'mkoffload's in that case should
get to the point where the latter is used.
> --- 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");
> }
Likewise.
> --- 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");
> }
Likewise.
> --- 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;
Should this be 'offload_abi_host_opts' for similarity with the
command-line option?
> @@ -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.");
I know we're not doing that consistently, but please use
'%<-foffload-abi-host-opts%>'.
> + 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=");
The option parsing in the 'mkoffload's is ad-hoc (not using the proepr
GCC infrastructure; which I'd like to change at some point in time...),
but let's please catch the case that '-foffload-abi-host-opts=[...]'
appears more than once (which could be necessary in certain
configurations, to produce ABI-compatible host code?). Not necessary to
implement that right now: for now, it'll be fine to 'fatal_error' if
running into a second '-foffload-abi-host-opts=[...]'.
Generally, likewise need to adjust 'gcc/config/gcn/mkoffload.cc'. I can
test this, or co-author, if you'd like.
> --- a/gcc/lto-wrapper.cc
> +++ b/gcc/lto-wrapper.cc
Don't we also need to adjust 'merge_and_complain':
case OPT_foffload_abi_:
if (existing_opt == -1)
decoded_options.safe_push (*foption);
else if (foption->value != decoded_options[existing_opt].value)
fatal_error (input_location,
"option %s not used consistently in all LTO input"
" files", foption->orig_option_with_args_text);
break;
> @@ -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:
Per my quick reading of the code, that should be correct.
> --- 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
| error_at (loc, "%<-foffload-abi%> option can be specified only for "
| "offload compiler");
| #endif
Please adjust the diagnostic. Surely the original option string will be
available for use with '%qs'.
Grüße
Thomas
