| Message ID | 1587c151-b99f-2f44-3044-7b01c296b9ad@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add 'default' to -foffload=; document that flag [PR67300] | expand |
On Thu, Jun 17, 2021 at 02:08:07PM +0200, Tobias Burnus wrote: > +@item -foffload=@var{offload-target-triplet} > +@itemx -foffload=@var{offload-target-triplet}=@var{flags} > +@itemx -foffload=@var{flags} > +@opindex foffload > +@cindex Offloading > +@cindex OpenACC > +@cindex OpenMP > +Specifies for which offload/non-host/accelerator devices code should > +be generated when using OpenACC (@option{-fopenacc}) -- or OpenMP > +(@option{-fopenmp}) with target regions. It additionally permits to > +pass arguments to the offload-target compiler. Note that code compiled > +for one or more offload-target devices can still be executable when some > +or all offload device are unavailable at runtime, in line with and as > +specified by the OpenACC and OpenMP specifications. I think default is not offload-target-triplet, so either we should rename the @var and then say that it is either offload-target-triplet or @code{default} or @code{disable}, or we should list those default and disable cases above as separate @itemx. > --- a/gcc/gcc.c > +++ b/gcc/gcc.c > @@ -4015,6 +4015,13 @@ handle_foffload_option (const char *arg) > break; > } > > + if (strcmp (target, "default") == 0) > + { > + free (offload_targets); > + offload_targets = xstrdup (OFFLOAD_TARGETS); > + break; > + } > + How does this interact with --enable-offload-defaulted ? Won't -fopenmp=default=-lm force in all configured targets even when say nvptx-none mkoffload/offloading compiler is missing? And how does it interact with previous -foffload=? I mean, -foffload=nvptx-none=-latomic -foffload=default=-lm Jakub
On 17.06.21 14:27, Jakub Jelinek via Gcc-patches wrote: > How does this interact with --enable-offload-defaulted ? Well, it requires all configured offload targets, making the installation mandatory. I think that's fine and consistent and is as documented. > Won't -fopenmp=default=-lm force in all configured targets even when say > nvptx-none mkoffload/offloading compiler is missing? Yes – but you could use -foffload=-lm instead if you don't want to have this. I have to admit that I missed that for -foffload=<target>=<options>, <target> can be a list of targets. Thus, '-foffload=default=-lm' probably should be supported. I think -foffload=disable=-... does not make sense & is now rejected, likewise for: -foffload=disable,default,disable,nvptx-none=-lm. For consistency, I now accept -foffload=default=-lm. And for consistency between -foffload=hsa,disable,nvptx-none and -foffload=hsa -foffload=disable -foffload=nvptx-none the first one no longer disables all offload devices but now acts like the second one and enables nvptx-none, only. I hope that everything is now consistent but I find -foffload= syntax wise overengineered. :-( * * * Updated version – only lightly tested. I think it is consistent like that and the documentation should now be comprehensive. (I will have to do some additional testing.) Further comments and thoughts? Tobias ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
On 6/17/21 10:03 AM, Tobias Burnus wrote: > Updated version – only lightly tested. I think it is > consistent like that and the documentation should now be > comprehensive. (I will have to do some additional testing.) > > Further comments and thoughts? 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 documentation to first have a paragraph discussing the default behavior, and then move on to how to modify it, with separate paragraphs for enabling offload targets explicitly and on adding options for offload compilation on all/some targets. I think it would useful to add an example with a target list and multiple options since I think the syntax looks pretty hairy. -Sandra
On Thu, Jun 17, 2021 at 11:41:39AM -0600, Sandra Loosemore wrote: > On 6/17/21 10:03 AM, Tobias Burnus wrote: > > > Updated version – only lightly tested. I think it is > > consistent like that and the documentation should now be > > comprehensive. (I will have to do some additional testing.) > > > > Further comments and thoughts? > > 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). We need to figure out what it means -foffload=nvptx-none -foffload=default, if the latter overrides the former (as if it wasn't specified), or if it adds all the remaining offload targets that are default in addition to it. 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. Jakub
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
On Thu, Jun 17, 2021 at 09:28:00PM +0200, Tobias Burnus wrote: > 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. Could we introduce a different option which wouldn't imply enabling that target: -foffload-options=<target triplet list>=option and make -foffload=<target triplet list>=option imply (expand in the driver) -foffload-options=<target triplet list>=option -foffload=<target triplet list> ? That would be mostly backwards compatible, but would allow users to specify options separately from the enabled target list. The <target triplet list> in the above cases couldn't include disable or default, but the -foffload=<target triplet list> case could, and disable (either in the list or separately) would simply disable all targets (even those enabled earlier), while default would reset the list to the default (basically cancel all previous non-options -foffload= options). And the -foffload-options= would accumulate in the order given on the command line. Jakub
On 6/17/21 1:40 PM, Jakub Jelinek wrote: > On Thu, Jun 17, 2021 at 09:28:00PM +0200, Tobias Burnus wrote: >> 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. > > Could we introduce a different option which wouldn't imply enabling that > target: > -foffload-options=<target triplet list>=option > and make > -foffload=<target triplet list>=option > imply (expand in the driver) > -foffload-options=<target triplet list>=option -foffload=<target triplet list> > ? > That would be mostly backwards compatible, but would allow users to specify options > separately from the enabled target list. > The <target triplet list> in the above cases couldn't include disable or > default, but the -foffload=<target triplet list> case could, and disable > (either in the list or separately) would simply disable all targets (even > those enabled earlier), while default would reset the list to the default > (basically cancel all previous non-options -foffload= options). > And the -foffload-options= would accumulate in the order given on the > command line. I don't feel qualified to comment on the details of the behavior, but separating the options and making them more orthogonal to one another would certainly make things easier to document. :-) One other thing I'd like to see in the docs is how to ask GCC what offload targets it is configured to support by default. This could be put in a paragraph that also includes the language about how you need to have the compilers for those offload targets installed too. -Sandra
On 17.06.21 23:57, Sandra Loosemore wrote: > On 6/17/21 1:40 PM, Jakub Jelinek wrote: >> Could we introduce a different option which wouldn't imply enabling that >> target: >> -foffload-options=<target triplet list>=option I think that works – in particular, if we do not document all the legacy stuff but just how it should be used. That includes not mentioning that disable and default can be used in a list and that those can also take arguments. > I don't feel qualified to comment on the details of the behavior, but > separating the options and making them more orthogonal to one another > would certainly make things easier to document. :-) I fully concur. Probably not fully polished, but I have attached a version for discussion. > One other thing I'd like to see in the docs is how to ask GCC what > offload targets it is configured to support by default. This could be > put in a paragraph that also includes the language about how you need > to have the compilers for those offload targets installed too. "gcc -v 2>&1 |grep OFFLOAD_TARGET_NAMES" shows them. Note that with the flags under discussion, you can modify the output, e.g. * "gcc -v -foffload=disable" removes it * "gcc -v -foffload=nvptx-none" either gives an error (not supported) or just shows that single target. * * * Installing: Building oneself: "make install" – and everything is there With a distributions, you need to find the name of the .rpm/.deb package for the target your are interested in and then run apt-get / yum / zypper / ... Hence, when you normally build GCC as a user, for all configured offload targets, their lto1 should be there - and if lto-wrapper cannot find it, it aborts with a fatal error. For distribution compilers, it usually just means that the optional package is not installed – the missing lto1 should/is then just ignored, That's the reason why GCC's configure script now has a flag --enable-offload-defaulted to toggle between supporting optional packages (distro use) and always errors (normal use). If you want to know how to build it, have a lot of fun with incomplete documentation and a look at https://gcc.gnu.org/wiki/Offloading or at the SUSE/Debian/Red Hat build scripts or g-t-s – and expect that you won't finish soon ... Tobias ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
On 6/17/21 5:05 PM, Tobias Burnus wrote: > On 17.06.21 23:57, Sandra Loosemore wrote: > >> On 6/17/21 1:40 PM, Jakub Jelinek wrote: >>> Could we introduce a different option which wouldn't imply enabling that >>> target: >>> -foffload-options=<target triplet list>=option > > I think that works – in particular, if we do not document all > the legacy stuff but just how it should be used. > > That includes not mentioning that disable and default can > be used in a list and that those can also take arguments. > >> I don't feel qualified to comment on the details of the behavior, but >> separating the options and making them more orthogonal to one another >> would certainly make things easier to document. :-) > > I fully concur. > > Probably not fully polished, but I have attached a version for discussion. Thanks. The description of the options is a lot easier to follow now, so I mostly have only nit-picky Texinfo/grammar/terminology comments about the docs now. > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index af2ce189fae..82993fa2c1d 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -204,6 +204,7 @@ in the following sections. > -fhosted -ffreestanding @gol > -fopenacc -fopenacc-dim=@var{geom} @gol > -fopenmp -fopenmp-simd @gol > +-foffload=@var{arg} -foffload-options=@var{arg} @gol > -fms-extensions -fplan9-extensions -fsso-struct=@var{endianness} @gol > -fallow-single-precision -fcond-mismatch -flax-vector-conversions @gol > -fsigned-bitfields -fsigned-char @gol Two spaces instead of one to separate options on the same line in a @gccoptlist environment. The -f options are alphabetized in most of the other @gccoptlist tables in the option summary section. I'm not sure why this group isn't, but you get extra credit if you fix that, too. > @@ -2639,6 +2640,46 @@ Enable handling of OpenMP's SIMD directives with @code{#pragma omp} > in C/C++ and @code{!$omp} in Fortran. Other OpenMP directives > are ignored. > > +@item -foffload=disable > +@itemx -foffload=default > +@itemx -foffload=@var{target-list} > +@opindex foffload > +@cindex Offloading > +@cindex OpenACC > +@cindex OpenMP "OpenACC" and "OpenMP" are far too general to be useful for @cindex entries. "Offloading targets", "OpenACC offloading targets", and "OpenMP offloading targets" would be more helpful. > +Specify for which offload targets code should be generated. By default, > +code for all supported targets is generated. Using > +@option{-foffload=disable} only code for the host fallback is > +generated, while @option{-foffload=}@var{target-list} generates code > +only for the specified comma-separated list of offload-targets triplets. > +To reset the value to the default, @option{-foffload=default} can be > +used. > + > +Note: Running the compiler with @option{-v} shows the list of > +configured offload targets under @code{OFFLOAD_TARGET_NAMES}. I'd like to rephrase this a little bit to avoid some awkward sentence construction, and also not introduce the term "triplet" without explanation in the user documentation (it's only used in the install and internals manuals). So: Specify for which OpenMP and OpenACC offload targets code should be generated. The default behavior, equivalent to @option{-foffload=default}, is to generate code for all supported offload targets. The @option{-foffload=disable} form generates code only for the host fallback, while @option{-foffload=@var{target-list}} generates code only for the specified comma-separated list of offload targets. Offload targets are specified in GCC's internal target-triplet format. You can run the compiler with @option{-v} to show the list of configured offload targets under @code{OFFLOAD_TARGET_NAMES}. > + > +@item -foffload-options=@var{options} > +@itemx -foffload-options=@var{target-triplet-list}=@var{options} > +@opindex foffload > +@cindex Offloading > +@cindex OpenACC > +@cindex OpenMP Ditto with the overly-general @cindex entries. I'd list these as "Offloading options" etc instead. Also use @var{target-list} here instead of @var{target-triplet-list} for consistency with the previous option and because triplets are not otherwise a user-visible thing in GCC. > + > +Specify the options passed to the offload-target compiler. While using > +@option{-foffload-options=}@var{options} passes the options to all enabled > +offloading compiler, > +@option{-foffload-options=}@var{target-triplet-list}=@var{options} can > +be used to pass them only to the specified comma-separated list of > +target triplets. > + Again to fix sentence construction and markup issues: With @option{-foffload-options=@var{options}}, GCC passes the specified @var{options} to the compilers for all enabled offloading targets. You can specify options that apply only to a specific target or targets by using the @option{-foffload-options=@var{target-list}=@var{options}} form. The @var{target-list} is a comma-separated list in the same format as for the @option{-foffload=} option. > +Typical commandlines are "command lines", two words. > + > +@smallexample > +-foffload=-lgfortran -foffload=-lm > +-foffload=-lm -foffload=nvptx-none=-latomic > +-foffload=amdgcn-amdhsa=-march=gfx906 -foffload=nvptx-none="-latomic -lm" > +@end smallexample > + > @item -fgnu-tm > @opindex fgnu-tm > When the option @option{-fgnu-tm} is specified, the compiler Thanks for working on this! :-) -Sandra
Hi Sandra, hi all, On 19.06.21 00:47, Sandra Loosemore wrote: > Thanks. The description of the options is a lot easier to follow now, > so I mostly have only nit-picky Texinfo/grammar/terminology comments > about the docs now. Thanks for your comments/wording suggestions. > The -f options are alphabetized in most of the other @gccoptlist > tables in the option summary section. I'm not sure why this group > isn't, but you get extra credit if you fix that, too. Done so. While doing so and then also sorting the list below, I noticed: * optlist still had -fallow-single-precision but the entry was removed in commit f458d1d5d7bd85e412689858ea5d5de681608fbb * there is -fgnu-tm - but it was missing from the optlist * I did not fully sort -fsigned-bitfields -funsigned-bitfields as those are in a single entry; hence, I also kept -fsigned-char -funsigned-char together. That also helps when reading as they belong together. >> +@smallexample >> +-foffload=-lgfortran -foffload=-lm I did notice that this should now be -foffload-option= ... – now fixed. Except for the doc/invoke.texi changes unchanged compared to previous version. OK? Tobias ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
I managed to delete the libgomp part before posting the patch, hence, reposted. (The change from -foffload= to -foffload-options= ensures that also other configured compilers such as GCN are used, an issue that Thomas found. The original -foffload=nvptx-none=-latomic was added because as otherwise the GCN part caused build issues for Richard.) Thus, this patch is like v3, except for the invoke.texi fixes suggested by Sandra (thanks!) + adding a ChangeLog and like v4, except the lost libgomp changes has been re-added (+ ChangeLog update). I hope it now is fine. Tobias ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
On 6/28/21 9:51 AM, Tobias Burnus wrote: > I managed to delete the libgomp part before posting the patch, hence, > reposted. > > (The change from -foffload= to -foffload-options= ensures that also > other configured compilers such as GCN are used, an issue that Thomas > found. The original -foffload=nvptx-none=-latomic was added because as > otherwise the GCN part caused build issues for Richard.) > > Thus, this patch is like v3, except for the invoke.texi fixes suggested > by Sandra (thanks!) + adding a ChangeLog > and like v4, except the lost libgomp changes has been re-added (+ > ChangeLog update). > > I hope it now is fine. Hmmm. > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -197,17 +197,17 @@ in the following sections. > > @item C Language Options > @xref{C Dialect Options,,Options Controlling C Dialect}. > -@gccoptlist{-ansi -std=@var{standard} -fgnu89-inline @gol > --fpermitted-flt-eval-methods=@var{standard} @gol > --aux-info @var{filename} -fallow-parameterless-variadic-functions @gol > --fno-asm -fno-builtin -fno-builtin-@var{function} -fgimple@gol > --fhosted -ffreestanding @gol > +@gccoptlist{-ansi -std=@var{standard} -aux-info @var{filename} @gol > +-fallow-parameterless-variadic-functions -fno-asm @gol > +-fno-builtin -fno-builtin-@var{function} -fcond-mismatch @gol > +-ffreestanding -fgimple -fgnu-tm -fgnu89-inline -fhosted @gol > +-flax-vector-conversions -fms-extensions @gol > -fopenacc -fopenacc-dim=@var{geom} @gol > +-foffload=@var{arg} -foffload-options=@var{arg} @gol Still need two spaces between these options on the same line inside @gccoptlist. > -fopenmp -fopenmp-simd @gol > --fms-extensions -fplan9-extensions -fsso-struct=@var{endianness} @gol > --fallow-single-precision -fcond-mismatch -flax-vector-conversions @gol > --fsigned-bitfields -fsigned-char @gol > --funsigned-bitfields -funsigned-char} > +-fpermitted-flt-eval-methods=@var{standard} @gol > +-fplan9-extensions -fsigned-bitfields -funsigned-bitfields @gol > +-fsigned-char -funsigned-char -fsso-struct=@var{endianness}} And on both the last two lines here. I didn't think it was necessary to alphabetize the actual documentation of the options (only the table in the option summary). I'll have to assume that you didn't actually change any of the text you moved around. The text for -foffload and -foffload-options looks fine now. The documentation part of the patch is OK with the whitespace changes (no need to post another version for me to review that). -Sandra
On Mon, Jun 28, 2021 at 05:51:30PM +0200, Tobias Burnus wrote: > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi I think it would be better to commit the reorderings in invoke.texi separately from the -foffload* changes, because otherwise people will keep wondering what actually really changed. It can go in before or after (and please take into account Sandra's review comments). > --- a/gcc/gcc.c > +++ b/gcc/gcc.c > @@ -3977,6 +3977,68 @@ driver_wrong_lang_callback (const struct cl_decoded_option *decoded, > static const char *spec_lang = 0; > static int last_language_n_infiles; > > + > +/* Check that GCC is configured to support the offload target. */ > + > +static void > +check_offload_target_name (const char *target, ptrdiff_t len) > +{ > + const char *n, *c = OFFLOAD_TARGETS; > + char *target2 = NULL; > + while (c) > + { > + n = strchr (c, ','); > + if (n == NULL) > + n = strchr (c, '\0'); > + if (len == n - c && strncmp (target, c, n - c) == 0) > + break; > + c = *n ? n + 1 : NULL; > + } > + if (!c) > + { > + if (target[len] != '\0') > + { > + target2 = XNEWVEC (char, len + 1); > + memcpy (target2, target, len); > + target2[len] = '\0'; > + } > + fatal_error (input_location, > + "GCC is not configured to support %qs as offload target", > + target2 ? target2 : target); Can't this be done without target2 with fatal_error (input_location, "GCC is not configured to support %q.*s as offload target", len, target); instead, regardless if target[len] is 0 or not? The message should be consistent between this function and handle_foffload_option (on the q in particular). Also, wonder if we shouldn't print the list of configured targets in that case, see candidates_list_and_hint functions and its callers. And it is unclear why we use fatal_error, can't unknown offload target names be simply ignored after emitting error? > + XDELETEVEC (target2); > + } > +} > + > +/* Sanity check for -foffload-options. */ > + > +static void > +check_foffload_target_names (const char *arg) > +{ > + const char *cur, *next, *end; > + /* If option argument starts with '-' then no target is specified and we > + do not need to parse it. */ > + if (arg[0] == '-') > + return; > + end = strchr (arg, '='); > + if (end == NULL) > + { > + error ("%<=options%> missing after %<-foffload-options=target%>"); Neither options nor target are keywords, so IMHO those shouldn't appear in between %< and %> but after the %>, so "%<=%>options missing after %%<-foffload-options=%>target" ? Otherwise LGTM. Jakub
First, the doc-sorting patch has now been applied separately as https://gcc.gnu.org/g:d479ddc0d9854905d03a3290b203a5dcb8db07eb On 29.06.21 13:58, Jakub Jelinek wrote: > Also, wonder if we shouldn't print the list of configured targets in that > case, see candidates_list_and_hint functions and its callers. > And it is unclear why we use fatal_error, can't unknown offload target names > be simply ignored after emitting error? Done so – as the changes now became a bit larger, I have attached the new version of the patch – despite the LGTM. Tobias ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf
On Tue, Jun 29, 2021 at 03:47:03PM +0200, Tobias Burnus wrote: > gcc/ChangeLog: > > * common.opt (-foffload=): Update description. > (-foffload-options=): New. > * doc/invoke.texi (C Language Options): Document > -foffload and -foffload-options. > * gcc.c (check_offload_target_name): New, split off from > handle_foffload_option. > (check_foffload_target_names): New. > (handle_foffload_option): Handle -foffload=default. > (driver_handle_option): Update for -foffload-options. > * lto-opts.c (lto_write_options): Use -foffload-options > instead of -foffload. > * lto-wrapper.c (merge_and_complain, append_offload_options): > Likewise. > * opts.c (common_handle_option): Likewise. > > libgomp/ChangeLog: > > * testsuite/libgomp.c-c++-common/reduction-16.c: Replace > -foffload=nvptx-none= by -foffload-options=nvptx-none= to > avoid disabling other offload targets. > * testsuite/libgomp.c-c++-common/reduction-5.c: Likewise. > * testsuite/libgomp.c-c++-common/reduction-6.c: Likewise. > * testsuite/libgomp.c/target-44.c: Likewise. Ok, thanks. Jakub
Hi Tobias, > On 29.06.21 13:58, Jakub Jelinek wrote: > >> Also, wonder if we shouldn't print the list of configured targets in that >> case, see candidates_list_and_hint functions and its callers. >> And it is unclear why we use fatal_error, can't unknown offload target names >> be simply ignored after emitting error? > > Done so – as the changes now became a bit larger, I have attached the > new version of the patch – despite the LGTM. this patch broke Solaris bootstrap (both 32 and 64-bit sparc and x86): /vol/gcc/src/hg/master/local/gcc/gcc.c: In function 'bool check_offload_target_name(const char*, ptrdiff_t)': /vol/gcc/src/hg/master/local/gcc/gcc.c:4010:23: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=] 4010 | cand[n - c] = '\0'; | ~~~~~~~~~~~~^~~~~~ In file included from /vol/gcc/src/hg/master/local/gcc/system.h:706, from /vol/gcc/src/hg/master/local/gcc/gcc.c:31: /vol/gcc/src/hg/master/local/gcc/../include/libiberty.h:733:36: note: at offset 1 into destination object of size 1 allocated by '__builtin_alloca' 733 | # define alloca(x) __builtin_alloca(x) | ~~~~~~~~~~~~~~~~^~~ /vol/gcc/src/hg/master/local/gcc/gcc.c:4000:29: note: in expansion of macro 'alloca' 4000 | char *cand = (char *) alloca (strlen (OFFLOAD_TARGETS) + 1); | ^~~~~~ Rainer
On Tue, Jun 29, 2021 at 10:48 PM Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > Hi Tobias, > > > On 29.06.21 13:58, Jakub Jelinek wrote: > > > >> Also, wonder if we shouldn't print the list of configured targets in > that > >> case, see candidates_list_and_hint functions and its callers. > >> And it is unclear why we use fatal_error, can't unknown offload target > names > >> be simply ignored after emitting error? > > > > Done so – as the changes now became a bit larger, I have attached the > > new version of the patch – despite the LGTM. > > this patch broke Solaris bootstrap (both 32 and 64-bit sparc and x86): > > /vol/gcc/src/hg/master/local/gcc/gcc.c: In function 'bool > check_offload_target_name(const char*, ptrdiff_t)': > /vol/gcc/src/hg/master/local/gcc/gcc.c:4010:23: error: writing 1 byte into > a region of size 0 [-Werror=stringop-overflow=] > 4010 | cand[n - c] = '\0'; > | ~~~~~~~~~~~~^~~~~~ > In file included from /vol/gcc/src/hg/master/local/gcc/system.h:706, > from /vol/gcc/src/hg/master/local/gcc/gcc.c:31: > /vol/gcc/src/hg/master/local/gcc/../include/libiberty.h:733:36: note: at > offset 1 into destination object of size 1 allocated by '__builtin_alloca' > 733 | # define alloca(x) __builtin_alloca(x) > | ~~~~~~~~~~~~~~~~^~~ > /vol/gcc/src/hg/master/local/gcc/gcc.c:4000:29: note: in expansion of > macro 'alloca' > 4000 | char *cand = (char *) alloca (strlen (OFFLOAD_TARGETS) + 1); > | ^~~~~~ > > Rainer > > Also seeing: FAIL: compiler driver --help=common option(s): "^ +-.*[^:.]$" absent from output: " -foffload=<targets>=<options> Specify options for the offloading targets" looks related to this patch. Thanks, Christophe
Hi Tobias, > this patch broke Solaris bootstrap (both 32 and 64-bit sparc and x86): > > /vol/gcc/src/hg/master/local/gcc/gcc.c: In function 'bool > check_offload_target_name(const char*, ptrdiff_t)': > /vol/gcc/src/hg/master/local/gcc/gcc.c:4010:23: error: writing 1 byte into > a region of size 0 [-Werror=stringop-overflow=] > 4010 | cand[n - c] = '\0'; > | ~~~~~~~~~~~~^~~~~~ > In file included from /vol/gcc/src/hg/master/local/gcc/system.h:706, > from /vol/gcc/src/hg/master/local/gcc/gcc.c:31: > /vol/gcc/src/hg/master/local/gcc/../include/libiberty.h:733:36: note: at offset 1 into destination object of size 1 allocated by '__builtin_alloca' > 733 | # define alloca(x) __builtin_alloca(x) > | ~~~~~~~~~~~~~~~~^~~ > /vol/gcc/src/hg/master/local/gcc/gcc.c:4000:29: note: in expansion of macro > 'alloca' > 4000 | char *cand = (char *) alloca (strlen (OFFLOAD_TARGETS) + 1); > | ^~~~~~ as of your next patch commit a3ce7d75dd9c0308b8565669f31127436cb2ba9f Author: Tobias Burnus <tobias@codesourcery.com> Date: Wed Jun 30 13:17:54 2021 +0200 gcc.c's check_offload_target_name: Fixes to inform hints Solaris bootstrap has been restored. Rainer
Add 'default' to -foffload=; document that flag [PR67300] It is long overdue to add documentation for -foffload; while in the past the need for the user to know the flag was limited, it is now needed rather often by users. Additionally, as soon as only some devices need specific flags such as -latomic, all non-specified offload targets get disabled, unless explicitly specified. To make that easier, the new flag -foffload=default has been added. That issue came up at https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570627.html PR other/67300 gcc/ChangeLog: * doc/invoke.texi (-foffload=): Finally add documentation for it. * gcc.c (handle_foffload_option): Support -foffload=default. libgomp/ChangeLog: * testsuite/libgomp.c-c++-common/reduction-16.c: Use -foffload=default. * testsuite/libgomp.c-c++-common/reduction-5.c: Likewise. * testsuite/libgomp.c-c++-common/reduction-6.c: Likewise. * testsuite/libgomp.c/target-44.c: Likewise. gcc/doc/invoke.texi | 40 +++++++++++++++++++++- gcc/gcc.c | 7 ++++ .../testsuite/libgomp.c-c++-common/reduction-16.c | 6 +++- .../testsuite/libgomp.c-c++-common/reduction-5.c | 7 +++- .../testsuite/libgomp.c-c++-common/reduction-6.c | 7 +++- libgomp/testsuite/libgomp.c/target-44.c | 6 +++- 6 files changed, 68 insertions(+), 5 deletions(-) diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index fe812cbd512..ab2ac0e6b3b 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -203,7 +203,7 @@ in the following sections. -fno-asm -fno-builtin -fno-builtin-@var{function} -fgimple@gol -fhosted -ffreestanding @gol -fopenacc -fopenacc-dim=@var{geom} @gol --fopenmp -fopenmp-simd @gol +-fopenmp -fopenmp-simd -foffload=@var{arg} @gol -fms-extensions -fplan9-extensions -fsso-struct=@var{endianness} @gol -fallow-single-precision -fcond-mismatch -flax-vector-conversions @gol -fsigned-bitfields -fsigned-char @gol @@ -2639,6 +2639,44 @@ Enable handling of OpenMP's SIMD directives with @code{#pragma omp} in C/C++ and @code{!$omp} in Fortran. Other OpenMP directives are ignored. +@item -foffload=@var{offload-target-triplet} +@itemx -foffload=@var{offload-target-triplet}=@var{flags} +@itemx -foffload=@var{flags} +@opindex foffload +@cindex Offloading +@cindex OpenACC +@cindex OpenMP +Specifies for which offload/non-host/accelerator devices code should +be generated when using OpenACC (@option{-fopenacc}) -- or OpenMP +(@option{-fopenmp}) with target regions. It additionally permits to +pass arguments to the offload-target compiler. Note that code compiled +for one or more offload-target devices can still be executable when some +or all offload device are unavailable at runtime, in line with and as +specified by the OpenACC and OpenMP specifications. + +The option @option{-foffload} can be specified multiple times and is +accumulative, except that @code{-foffload=disable} disables the code +generation for all offload-target devices that were enabled before that +option. By default, code for all supported offload devices is generated; +however, as soon as any @option{-foffload} with a target triplet has +been specified, code is only generated for those target triplets +that appeared in an @option{-foffload} option - that is independent of +whether only the triplet or triplet plus flags have been used. By +specifying @code{-foffload=default} in addition, the code generation +for all supported devices is enabled. Note that GCC can be configured +such that non-installed offload compilers are ignored; in that case, +using @code{-foffload=default} enforces the compilation with all +configured devices, which makes the installation of all offload +compilers mandatory. + +The flags specified with @option{-foffload=}@var{flags} are passed to +the all enabled offloading compilers; whereas using +@option{-foffload=}@var{offload-target-triplet}=@var{flags}, the flags +are only passed to the specified compiler. Typical applications is to +pass target-specific flags to the offloading compiler or to link +libraries required for the offloaded code such as @code{-lm}, +@code{-latomic}, or @code{-lgfortran}. + @item -fgnu-tm @opindex fgnu-tm When the option @option{-fgnu-tm} is specified, the compiler diff --git a/gcc/gcc.c b/gcc/gcc.c index af286400a4a..73385b56a89 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -4015,6 +4015,13 @@ handle_foffload_option (const char *arg) break; } + if (strcmp (target, "default") == 0) + { + free (offload_targets); + offload_targets = xstrdup (OFFLOAD_TARGETS); + break; + } + /* Check that GCC is configured to support the offload target. */ c = OFFLOAD_TARGETS; while (c) diff --git a/libgomp/testsuite/libgomp.c-c++-common/reduction-16.c b/libgomp/testsuite/libgomp.c-c++-common/reduction-16.c index 0eea73b144b..669f5d5b1af 100644 --- a/libgomp/testsuite/libgomp.c-c++-common/reduction-16.c +++ b/libgomp/testsuite/libgomp.c-c++-common/reduction-16.c @@ -1,5 +1,9 @@ /* { dg-do run } */ -/* { dg-additional-options "-foffload=nvptx-none=-latomic" { target offload_target_nvptx } } */ +/* { dg-additional-options "-foffload=default -foffload=nvptx-none=-latomic" { target offload_target_nvptx } } */ +/* nvptx often needs -latomic (like for this testcase), which is not linked + automatically. However, when the compiler supports the code generation for + multiple offload targets including nvptx, the -foffload=default is needed as + otherwise the code generation for all other no-host devices is disabled. */ #include <stdlib.h> diff --git a/libgomp/testsuite/libgomp.c-c++-common/reduction-5.c b/libgomp/testsuite/libgomp.c-c++-common/reduction-5.c index 31fa2670312..95fc1b79eed 100644 --- a/libgomp/testsuite/libgomp.c-c++-common/reduction-5.c +++ b/libgomp/testsuite/libgomp.c-c++-common/reduction-5.c @@ -1,4 +1,9 @@ -/* { dg-additional-options "-foffload=nvptx-none=-latomic" { target { offload_target_nvptx } } } */ +/* { dg-additional-options "-foffload=default -foffload=nvptx-none=-latomic" { target { offload_target_nvptx } } } */ +/* nvptx often needs -latomic (like for this testcase), which is not linked + automatically. However, when the compiler supports the code generation for + multiple offload targets including nvptx, the -foffload=default is needed as + otherwise the code generation for all other no-host devices is disabled. */ + /* C / C++'s logical AND and OR operators take any scalar argument which compares (un)equal to 0 - the result 1 or 0 and of type int. diff --git a/libgomp/testsuite/libgomp.c-c++-common/reduction-6.c b/libgomp/testsuite/libgomp.c-c++-common/reduction-6.c index 727e11e4edf..ba2c9bdc2a8 100644 --- a/libgomp/testsuite/libgomp.c-c++-common/reduction-6.c +++ b/libgomp/testsuite/libgomp.c-c++-common/reduction-6.c @@ -1,4 +1,9 @@ -/* { dg-additional-options "-foffload=nvptx-none=-latomic" { target { offload_target_nvptx } } } */ +/* { dg-additional-options "-foffload=default -foffload=nvptx-none=-latomic" { target { offload_target_nvptx } } } */ +/* nvptx often needs -latomic (like for this testcase), which is not linked + automatically. However, when the compiler supports the code generation for + multiple offload targets including nvptx, the -foffload=default is needed as + otherwise the code generation for all other no-host devices is disabled. */ + /* C / C++'s logical AND and OR operators take any scalar argument which compares (un)equal to 0 - the result 1 or 0 and of type int. diff --git a/libgomp/testsuite/libgomp.c/target-44.c b/libgomp/testsuite/libgomp.c/target-44.c index b95e807a114..9659fe3ee46 100644 --- a/libgomp/testsuite/libgomp.c/target-44.c +++ b/libgomp/testsuite/libgomp.c/target-44.c @@ -1,4 +1,8 @@ -/* { dg-additional-options "-foffload=nvptx-none=-latomic" { target { offload_target_nvptx } } } */ +/* { dg-additional-options "-foffload=default -foffload=nvptx-none=-latomic" { target { offload_target_nvptx } } } */ +/* nvptx often needs -latomic (like for this testcase), which is not linked + automatically. However, when the compiler supports the code generation for + multiple offload targets including nvptx, the -foffload=default is needed as + otherwise the code generation for all other no-host devices is disabled. */ #include <stdlib.h>