Re: [Patch] Add 'default' to -foffload=; document that flag [PR67300]

[Tobias Burnus <tobias@codesourcery.com>]

On 17.06.21 19:50, Jakub Jelinek wrote:

> On Thu, Jun 17, 2021 at 11:41:39AM -0600, Sandra Loosemore wrote:
>> I think it would useful to add an example with a target list and
>> multiple options since I think the syntax looks pretty hairy.

I fully concur that -foffload= is a mess trying to achieve too many
things at the same time. [Hence, I kept rephrasing the description
without reaching a good wording.]

In terms of real-world usage, I think something like the following
covers all real-world needs:

-foffload=disable
-foffload=-latomic -foffload=-lm
-foffload="-lgfortran -lm" -foffload=nvptx-none=-latomic
-foffload=-lm -foffload=nvptx-none=-latomic -foffload=amdgcn-amdhsa=-march=gfx906
-foffload=-fdump-tree-all

Possibly with -foffload=default added to item 3 + 4, but cf. below.

>> Hmmm, I had started to put together some comments on
>> grammar/punctuation/markup on the first version before the second iteration
>> showed up in my mailbox, but more critically I could not figure out whether
>> -foffload=default is supposed to be exactly identical to the default
>> behavior; if it isn't, it should be, or -foffload=default ought to be
>> renamed.  So let's get that sorted out first.  I suggest reorganizing the
> Yeah.  If we want for --enable-offload-default also all configured targets,
> we could add another keyword for it (all), but I'm not sure it would be
> useful, because whenever it would be different from default it would mean
> the linking would fail because one or more offloading targets that were
> configured isn't supported (installed).

I am not sure whether I fully agree with this or not. However:

Let's propose something radical, which probably won't break any real-world
code, avoids the need to add a new -foffload=<something> keyword and is
also intuitive to the user:

* -foffload=<target triplet list>=-option

Suggestion: This no longer affects the list of enabled targets. As by default
all targets are enabled, this one will (kept) be(en) enabled (but might
silently fail if the target lto1 is not installed).

* -foffload=disable  and -foffload=<triplet-list>

This is the only way to modify the list of supported offload devices to those
specified. By adding a triplet explicitly, it will give an error via lto1.

That will solve all issues, possibly except for
   -foffload=-lm -foffload=nxpt-none=-latomic -foffload=amdgcn-amdhsa
some might find it surprising that nvptx offloading will be disabled,
but others might find it natural.


Hence: Do you think this change makes sense? It looks somewhat consistent,
avoids a new -foffload=<something> flag etc.?
It also would solve the testsuite issue without needing to add
a new flag and a comment explaining why the flag is needed.


What do you think? It breaks backward compatibility, but I am not
sure anyone way relying on the current behavior.

   * * *

Compared to the current version, I would also give an error if
  -foffload=<list>
contains 'disable' and any other target entry (including 'disable')
and for '-foffload=disable=<options> – as both does not make any sense.

This does not really change the semantic but avoids odd code.


-foffload=<triplet list> will then restrict it to certain
targets & by specifying them explicitly, lto1 will still give
an error when not available.

> And similarly figure out what happens with the optional flags, if they are
> gathered from all the -foffload= options that refer to a particular target,
> or taken from the last -foffload option that mentions that target, something
> else.

Currently,
   -foffload=-lm -foffload=nvptx-none=-latomic -foffload=-lgfortran
works, taking the arguments "-lm -lgfortran" for all but nvptx-none
and "-lm -latomic -lgfortran" for nvptx-none. I think this really needs
to continue to work.

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf