On Wed, Jan 8, 2020 at 11:20 AM Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> wrote: > > On Tue, 5 Nov 2019 at 17:38, Richard Biener <richard.guent...@gmail.com> > wrote: > > > > On Tue, Nov 5, 2019 at 12:17 AM Kugan Vivekanandarajah > > <kugan.vivekanandara...@linaro.org> wrote: > > > > > > Hi, > > > Thanks for the review. > > > > > > On Tue, 5 Nov 2019 at 03:57, H.J. Lu <hjl.to...@gmail.com> wrote: > > > > > > > > On Sun, Nov 3, 2019 at 6:45 PM Kugan Vivekanandarajah > > > > <kugan.vivekanandara...@linaro.org> wrote: > > > > > > > > > > Thanks for the reviews. > > > > > > > > > > > > > > > On Sat, 2 Nov 2019 at 02:49, H.J. Lu <hjl.to...@gmail.com> wrote: > > > > > > > > > > > > On Thu, Oct 31, 2019 at 6:33 PM Kugan Vivekanandarajah > > > > > > <kugan.vivekanandara...@linaro.org> wrote: > > > > > > > > > > > > > > On Wed, 30 Oct 2019 at 03:11, H.J. Lu <hjl.to...@gmail.com> wrote: > > > > > > > > > > > > > > > > On Sun, Oct 27, 2019 at 6:33 PM Kugan Vivekanandarajah > > > > > > > > <kugan.vivekanandara...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > Hi Richard, > > > > > > > > > > > > > > > > > > Thanks for the review. > > > > > > > > > > > > > > > > > > On Wed, 23 Oct 2019 at 23:07, Richard Biener > > > > > > > > > <richard.guent...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > On Mon, Oct 21, 2019 at 10:04 AM Kugan Vivekanandarajah > > > > > > > > > > <kugan.vivekanandara...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Richard, > > > > > > > > > > > > > > > > > > > > > > Thanks for the pointers. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, 11 Oct 2019 at 22:33, Richard Biener > > > > > > > > > > > <richard.guent...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Oct 11, 2019 at 6:15 AM Kugan Vivekanandarajah > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Richard, > > > > > > > > > > > > > Thanks for the review. > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2 Oct 2019 at 20:41, Richard Biener > > > > > > > > > > > > > <richard.guent...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Oct 2, 2019 at 10:39 AM Kugan > > > > > > > > > > > > > > Vivekanandarajah > > > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > As mentioned in the PR, attached patch adds > > > > > > > > > > > > > > > COLLECT_AS_OPTIONS for > > > > > > > > > > > > > > > passing assembler options specified with -Wa, to > > > > > > > > > > > > > > > the link-time driver. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The proposed solution only works for uniform -Wa > > > > > > > > > > > > > > > options across all > > > > > > > > > > > > > > > TUs. As mentioned by Richard Biener, supporting > > > > > > > > > > > > > > > non-uniform -Wa flags > > > > > > > > > > > > > > > would require either adjusting partitioning > > > > > > > > > > > > > > > according to flags or > > > > > > > > > > > > > > > emitting multiple object files from a single > > > > > > > > > > > > > > > LTRANS CU. We could > > > > > > > > > > > > > > > consider this as a follow up. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Bootstrapped and regression tests on > > > > > > > > > > > > > > > arm-linux-gcc. Is this OK for trunk? > > > > > > > > > > > > > > > > > > > > > > > > > > > > While it works for your simple cases it is unlikely > > > > > > > > > > > > > > to work in practice since > > > > > > > > > > > > > > your implementation needs the assembler options be > > > > > > > > > > > > > > present at the link > > > > > > > > > > > > > > command line. I agree that this might be the way > > > > > > > > > > > > > > for people to go when > > > > > > > > > > > > > > they face the issue but then it needs to be > > > > > > > > > > > > > > documented somewhere > > > > > > > > > > > > > > in the manual. > > > > > > > > > > > > > > > > > > > > > > > > > > > > That is, with COLLECT_AS_OPTION (why singular? I'd > > > > > > > > > > > > > > expected > > > > > > > > > > > > > > COLLECT_AS_OPTIONS) available to cc1 we could > > > > > > > > > > > > > > stream this string > > > > > > > > > > > > > > to lto_options and re-materialize it at link time > > > > > > > > > > > > > > (and diagnose mismatches > > > > > > > > > > > > > > even if we like). > > > > > > > > > > > > > OK. I will try to implement this. So the idea is if > > > > > > > > > > > > > we provide > > > > > > > > > > > > > -Wa,options as part of the lto compile, this should > > > > > > > > > > > > > be available > > > > > > > > > > > > > during link time. Like in: > > > > > > > > > > > > > > > > > > > > > > > > > > arm-linux-gnueabihf-gcc -march=armv7-a -mthumb -O2 > > > > > > > > > > > > > -flto > > > > > > > > > > > > > -Wa,-mimplicit-it=always,-mthumb -c test.c > > > > > > > > > > > > > arm-linux-gnueabihf-gcc -flto test.o > > > > > > > > > > > > > > > > > > > > > > > > > > I am not sure where should we stream this. Currently, > > > > > > > > > > > > > cl_optimization > > > > > > > > > > > > > has all the optimization flag provided for compiler > > > > > > > > > > > > > and it is > > > > > > > > > > > > > autogenerated and all the flags are integer values. > > > > > > > > > > > > > Do you have any > > > > > > > > > > > > > preference or example where this should be done. > > > > > > > > > > > > > > > > > > > > > > > > In lto_write_options, I'd simply append the contents of > > > > > > > > > > > > COLLECT_AS_OPTIONS > > > > > > > > > > > > (with -Wa, prepended to each of them), then recover > > > > > > > > > > > > them in lto-wrapper > > > > > > > > > > > > for each TU and pass them down to the LTRANS compiles > > > > > > > > > > > > (if they agree > > > > > > > > > > > > for all TUs, otherwise I'd warn and drop them). > > > > > > > > > > > > > > > > > > > > > > Attached patch streams it and also make sure that the > > > > > > > > > > > options are the > > > > > > > > > > > same for all the TUs. Maybe it is a bit restrictive. > > > > > > > > > > > > > > > > > > > > > > What is the best place to document COLLECT_AS_OPTIONS. We > > > > > > > > > > > don't seem > > > > > > > > > > > to document COLLECT_GCC_OPTIONS anywhere ? > > > > > > > > > > > > > > > > > > > > Nowhere, it's an implementation detail then. > > > > > > > > > > > > > > > > > > > > > Attached patch passes regression and also fixes the > > > > > > > > > > > original ARM > > > > > > > > > > > kernel build issue with tumb2. > > > > > > > > > > > > > > > > > > > > Did you try this with multiple assembler options? I see > > > > > > > > > > you stream > > > > > > > > > > them as -Wa,-mfpu=xyz,-mthumb but then compare the whole > > > > > > > > > > option strings so a mismatch with -Wa,-mthumb,-mfpu=xyz > > > > > > > > > > would be > > > > > > > > > > diagnosed. If there's a spec induced -Wa option do we get > > > > > > > > > > to see > > > > > > > > > > that as well? I can imagine -march=xyz enabling a -Wa > > > > > > > > > > option > > > > > > > > > > for example. > > > > > > > > > > > > > > > > > > > > + *collect_as = XNEWVEC (char, strlen > > > > > > > > > > (args_text) + 1); > > > > > > > > > > + strcpy (*collect_as, args_text); > > > > > > > > > > > > > > > > > > > > there's strdup. Btw, I'm not sure why you don't simply > > > > > > > > > > leave > > > > > > > > > > the -Wa option in the merged options [individually] and > > > > > > > > > > match > > > > > > > > > > them up but go the route of comparing strings and carrying > > > > > > > > > > that > > > > > > > > > > along separately. I think that would be much better. > > > > > > > > > > > > > > > > > > Is attached patch which does this is OK? > > > > > > > > > > > > > > > > > > > > > > > > > Don't you need to also handle -Xassembler? Since -Wa, doesn't > > > > > > > > work with comma in > > > > > > > > assembler options, like -mfoo=foo1,foo2, one needs to use > > > > > > > > > > > > > > > > -Xassembler -mfoo=foo1,foo2 > > > > > > > > > > > > > > > > to pass -mfoo=foo1,foo2 to assembler. > > > > > > > > > > > > > > > > > > > > > gcc -flto -O2 -Wa,-mcpu=zzz1 -mcpu=xxx1 -c foo.c > > > > > > > gcc -flto -O2 -Wa,-mcpu=zzz2 -mcpu=xxx2 -c bar.c > > > > > > > > > > > > > > What should be the option we should provide for the final > > > > > > > gcc -flto foo.o bar.o -o out > > > > > > > > > > > > > > I think our ultimate aim is to handle this in LTO partitioning. > > > > > > > That > > > > > > > is, we should create partitioning such that each partition has the > > > > > > > same -Wa options. This could also handle -Xassembler > > > > > > > -mfoo=foo1,foo2 > > > > > > > which H.J. Lu wanted. We need to modify the heuristics and do some > > > > > > > performance testing. > > > > > > > > > > > > > > In the meantime we could push a simpler solution which is to > > > > > > > accept > > > > > > > -Wa option if they are identical. This would fix at least some of > > > > > > > the > > > > > > > reported cases. Trying to work out what is compatible options, > > > > > > > even if > > > > > > > we ask the back-end to do this, is not a straightforward strategy > > > > > > > and > > > > > > > can be a maintenance nightmare. Unless we can query GNU AS > > > > > > > somehow. If > > > > > > > I am missing something please let me know. > > > > > > > > > > > > +/* Store switches specified for as with -Wa in COLLECT_AS_OPTIONS > > > > > > + and place that in the environment. */ > > > > > > +static void > > > > > > +putenv_COLLECT_AS_OPTIONS (vec<char_p> vec) > > > > > > +{ > > > > > > + unsigned ix; > > > > > > + char *opt; > > > > > > + int len = vec.length (); > > > > > > + > > > > > > + if (!len) > > > > > > + return; > > > > > > + > > > > > > + obstack_init (&collect_obstack); > > > > > > + obstack_grow (&collect_obstack, "COLLECT_AS_OPTIONS=", > > > > > > + sizeof ("COLLECT_AS_OPTIONS=") - 1); > > > > > > + obstack_grow (&collect_obstack, "-Wa,", strlen ("-Wa,")); > > > > > > + > > > > > > + FOR_EACH_VEC_ELT (vec, ix, opt) > > > > > > + { > > > > > > + obstack_grow (&collect_obstack, opt, strlen (opt)); > > > > > > + --len; > > > > > > + if (len) > > > > > > + obstack_grow (&collect_obstack, ",", strlen (",")); > > > > > > + } > > > > > > + > > > > > > + xputenv (XOBFINISH (&collect_obstack, char *)); > > > > > > > > > > > > This missed the null terminator. > > > > > > > > > > Attached patch addresses the review comments I got so far. > > > > > > > > > > > > > + if (len) > > > > + obstack_grow (&collect_obstack, ",", strlen (",")); > > > > > > > > Why not sizeof (",") - 1? > > > I guess I copied and pasted it from elsewhere else. We seem to use > > > both. I have changed it now. > > > > > > > > > > > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c > > > > index 9a7bbd0c022..148c52906d1 100644 > > > > --- a/gcc/lto-wrapper.c > > > > +++ b/gcc/lto-wrapper.c > > > > @@ -253,6 +253,11 @@ merge_and_complain (struct cl_decoded_option > > > > **decoded_options, > > > > break; > > > > > > > > default: > > > > + if (foption->opt_index == OPT_Wa_) > > > > + { > > > > + append_option (decoded_options, decoded_options_count, foption); > > > > + break; > > > > + } > > > > if (!(cl_options[foption->opt_index].flags & CL_TARGET)) > > > > break; > > > > > > > > Why not use "case OPT_Wa_:" here? > > > Done. > > > > > > > > For > > > > > > > > + static const char *collect_as; > > > > + for (unsigned int j = 1; j < count; ++j) > > > > + { > > > > + struct cl_decoded_option *option = &opts[j]; > > > > + if (j == 1) > > > > + collect_as = NULL; > > > > > > > > why not simply > > > > > > > > const char *collect_as = NULL? > > > > > > I wanted to make sure that if we call this from multiple places, it > > > still works. I guess it is still going to be the same. I have changed > > > it now as you have suggested. > > > > > > Is this revised patch OK? I will do a fresh bootstrap and regression > > > testing before committing. > > > > In putenv_COLLECT_AS_OPTIONS you'll happily make > > -Wa,-march=foo,bar out of -Xassembler -march=foo,bar which > > will later cause us to fail to assemble with unknown assembler options. > > May I suggest to instead always use -Xassembler syntax in > > COLLECT_AS_OPTIONS? Please also make sure to quote > > options the same way set_collect_gcc_options does > > (with '', separated by spaces). Then the lto-opts.c part > > becomes "easier" as you can simply copy the string to the > > obstack without wrapping it again with append_to_collect_gcc_options. > > > > In lto-wrapper you then only have to handle OPT_Xassembler. > > > > You simply end up appending _all_ assembler options in order > > of TUs processed by lto-wrapper to the final command (N times > > even if exactly the same). I'm also not sure how you can check > > for positional equivalence (or if we even should). With -Wa > > we could compare the full option string but with separate -Xassembler > > we're having a more difficult task here. OTOH your patch doesn't > > do any comparing here. > > > > Your append_compiler_wa_options should be merged into > > append_compiler_options, passing -Xassembler through. > Hi Richard, > Since Kugan has left Linaro (and GCC), I'd like to take up this task. > I have modified his patch to always pass assembler options via -Xassembler. > Does it look OK ? > > I am not sure how we should proceed with error-checking for Xassembler ? > In lto-wrapper, I suppose, we can append all Xassembler options for a > TU into a single string, and then > do strcmp similar to previous patch(es) doing strcmp for -Wa options > string, although not sure if that's a good idea.
I think there are multiple issues with the main one being how to actually interpret -Xassembler in the LTO context. First let me point out some bits in the COLLECT_AS_OPTIONS parts. + FOR_EACH_VEC_ELT (vec, ix, opt) + { + obstack_grow (&collect_obstack, "\'-Xassembler\' ", + strlen ("\'-Xassembler\' ")); quoting of -Xassembler is not necessary. + obstack_1grow (&collect_obstack, '\''); + obstack_grow (&collect_obstack, opt, strlen (opt)); + obstack_grow (&collect_obstack, "\' ", 2); This adds a stray space after the last option. Note that COLLECT_AS_OPTIONS gives the impression of listing assembler options but the above adds GCC driver options - assembler options prepended by -Xassembler. IMHO we should drop the -Xassembler emission from the above loop and simply emit the plain assembler options. That requires adjustments to lto_write_options, adding those -Xassembler options. + char *asm_opts = XOBFINISH (&collect_obstack, char *); + xputenv (XOBFINISH (&collect_obstack, char *)); + xputenv (asm_opts); That outputs the ENV twice. Note that we record things like --version or --help into assembler_options but I'm not sure the merging of assembler options should be affected on whether one TU was compiled with -v or not. This might mean simply pruning those in lto-options.c (not listing them in COLLECT_AS_OPTIONS wouldn't tell the truth). @@ -252,6 +252,10 @@ merge_and_complain (struct cl_decoded_option **decoded_options, case OPT_SPECIAL_input_file: break; + case OPT_Xassembler: + append_option (decoded_options, decoded_options_count, foption); + break; + this adds the same option over-and-over again, possibly becoming unwieldly. Most of the function also assumes that option position isn't important which might or might not be true. So I think a better course of action would be to not handle Xassembler in the above loop but do a separate one checking 1:1 equality of passed assembler options like /* Verify -Xassembler options are the same on all TUs. */ j = 0; i = 0; unsigned Xascount = 0; while (j < *decoded_options_count && i < fdeconded_options_count) { while (fdecoded_options[i].opt_index != OPT_Xassembler) ++i; same for *decoded_options if (stray Xassembler on one side) fatal_error (...); if (strcmp (...) != 0) fatal_error (...); } which means we use the -Xassembler options from the first TU and above only verify those match those from all other TUs. Richard. > Thanks, > Prathamesh > > > > Thanks, > > Richard. > > > > > Thanks, > > > Kugan > > > > > > > > > > > > > > > H.J.