> -----Original Message----- > From: Thomas Schwinge > Sent: Friday, September 6, 2024 2:31 PM > To: Prathamesh Kulkarni ; Richard Biener > > Cc: Andrew Pinski ; gcc-patches@gcc.gnu.org; Jakub > Jelinek > Subject: RE: [nvptx] Pass -m32/-m64 to host_compiler if it has > multilib support > > External email: Use caution opening links or attachments > > > Hi! Hi Thomas, Thanks for the review! > > On 2024-08-16T15:36:29+0000, Prathamesh Kulkarni > wrote: > >> > Am 13.08.2024 um 17:48 schrieb Thomas Schwinge > >> : > >> > On 2024-08-12T07:50:07+0000, Prathamesh Kulkarni > >> wrote: > >> >>> From: Thomas Schwinge > >> >>> Sent: Friday, August 9, 2024 12:55 AM > >> > > >> >>> On 2024-08-08T06:46:25-0700, Andrew Pinski > >> wrote: > >> >>>> On Thu, Aug 8, 2024 at 6:11 AM Prathamesh Kulkarni > >> >>>> 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 > >> 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== Specify host abi > options. > > + > > Here, 'option or option=abi' and '=' should be just > 'options' and '', right? And, TAB between '-foffload-abi- > host-opts=' and its help text. And upper-case ABI. Yes right, sorry. Fixed in the attached patch. > > > --- 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. Um, would that still possibly result in arch mismatch for host objects and xnvptx-none.o if we don't pass host ABI opts for ILP32 ? For eg, if the host compiler defaults to 64-bit code-gen (and user requests for 32-bit code gen on host), and we avoid passing host ABI opts for -foffload-abi=ilp32, it will generate 64-bit xnvptx-none.o (corresponding to empty ptx_cfile_name), while rest of the host objects will be 32-bit, or am I misunderstanding ? The attached patch avoids passing -foffload-abi-host-opts if -foffload-abi=ilp32. > > > --- 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? Fixed, thanks. > > > @@ -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%>'. Done. > > > + 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=[...]'. Done. > > Generally, likewise need to adjust 'gcc/config/gcn/mkoffload.cc'. I > can test this, or co-author, if you'd like. Done. > > > --- a/gcc/lto-wrapper.cc > > +++ b/gcc/lto-wrapper.cc > > Don't we also need to adjust 'merge_and_complain': Done. > > 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 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'. Done, thanks. I verified the patch survives libgomp testing for Aarch64/nvptx offloading. Could you please test the patch for gcn backend ? Thanks! Signed-off-by: Prathamesh Kulkarni Thanks, Prathamesh > > > Grüße > Thomas