Hi Prathamesh!
On 2024-09-09T06:31:18+0000, Prathamesh Kulkarni <prathame...@nvidia.com> wrote:
>> -----Original Message-----
>> From: Thomas Schwinge <tschwi...@baylibre.com>
>> Sent: Friday, September 6, 2024 2:31 PM
>> 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:
>> >> >> 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).
>> > --- 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.
Oh... I was wrong with the latter item: I failed to see that the
'mkoffload's still do 'compile_native' even if they don't create an
actual offload image, sorry!
> 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 ?
You're quite right -- my fault.
> The attached patch avoids passing -foffload-abi-host-opts if
> -foffload-abi=ilp32.
So, sorry for the back and forth. I think we now agree that we do need
'-foffload-abi-host-opts=[...]' specified in call cases (as you
originally had), and then again unconditionally use
'offload_abi_host_opts' in the 'mkoffload's' 'compile_native' functions.
> Could you please test the patch for gcn backend ?
I'll do that.
> [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
"its", by the way. ;-)
> to host_compiler.
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> +foffload-abi-host-opts=
> +Common Driver Joined MissingArgError(option missing after %qs)
> +-foffload-abi-host-opts=<options> Specify host ABI options.
> +
Still need TAB between '-foffload-abi-host-opts=<options>' and its help
text.
> --- a/gcc/config/gcn/mkoffload.cc
> +++ b/gcc/config/gcn/mkoffload.cc
> @@ -998,6 +996,14 @@ main (int argc, char **argv)
> "unrecognizable argument of option %<" STR "%>");
> }
> #undef STR
> + else if (startswith (argv[i], "-foffload-abi-host-opts="))
> + {
> + if (offload_abi_host_opts)
> + fatal_error (input_location,
> + "-foffload-abi-host-opts specified multiple times");
ACK, but again '%<-foffload-abi-host-opts%>', please. (May also use
another '#define STR "[...]"' for the duplicated string, but I don't
care.)
> --- a/gcc/config/nvptx/mkoffload.cc
> +++ b/gcc/config/nvptx/mkoffload.cc
> @@ -721,6 +718,14 @@ main (int argc, char **argv)
> "unrecognizable argument of option " STR);
> }
> #undef STR
> + else if (startswith (argv[i], "-foffload-abi-host-opts="))
> + {
> + if (offload_abi_host_opts)
> + fatal_error (input_location,
> + "-foffload-abi-host-opts specified multiple times");
Likewise.
> --- a/gcc/lto-wrapper.cc
> +++ b/gcc/lto-wrapper.cc
> @@ -484,6 +484,7 @@ merge_and_complain (vec<cl_decoded_option>
> &decoded_options,
>
>
> case OPT_foffload_abi_:
> + case OPT_foffload_abi_host_opts_:
> if (existing_opt == -1)
> decoded_options.safe_push (*foption);
> else if (foption->value != decoded_options[existing_opt].value)
> @@ -745,6 +746,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:
I'm not too familiar with this code, but that now looks right to me.
> --- a/gcc/opts.cc
> +++ b/gcc/opts.cc
> @@ -3070,11 +3070,12 @@ 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");
> + error_at (loc, "%qs option can be specified only for "
> + "offload compiler", arg);
> #endif
> break;
With this, using '-foffload-abi=ilp32' with the host compiler results in:
cc1: error: ‘ilp32’ option can be specified only for offload compiler
..., or for '-foffload-abi-host-opts=-m64' in:
xgcc: error: ‘-m64’ option can be specified only for offload compiler
..., so 'arg' is only the option argument, not the whole string.
And, incidentally, 'cc1' vs. 'xgcc' means without vs. with 'Driver'
option property (re your 'gcc/common.opt' change). Which should it be?
'-foffload-abi=[...]' currently doesn't have 'Driver', so probably
'-foffload-abi-host-opts=[...]' also shouldn't?
With those small items addressed, the patch looks good to me, thanks!
(..., and I'll still test GCN offloading.)
Grüße
Thomas
