On Fri, Nov 8, 2019 at 3:35 AM Kugan Vivekanandarajah
<kugan.vivekanandara...@linaro.org> wrote:
>
> Hi Richard,
> Thanks for the review.
>
> On Tue, 5 Nov 2019 at 23:08, 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.
>
>
> I changes it to look:
> 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);
>
> FOR_EACH_VEC_ELT (vec, ix, opt)
> {
> obstack_grow (&collect_obstack, "-Xassembler ",
> sizeof ("-Xassembler ") - 1);
> obstack_grow (&collect_obstack, opt, strlen (opt));
> }
>
> obstack_1grow (&collect_obstack, '\0');
> xputenv (XOBFINISH (&collect_obstack, char *));
> }
>
> But when I use it,
> gcc -flto f.c -Wa,-mimplicit-it=always -c
> or gcc -flto f.c -Xassembler -mimplicit-it=always -c
>
> gcc -flto l.o f.o
> foptions->original_option_with_args_text "-Xassembler
> -mimplicit-it=always" is decoded as 1493 (OPT_SPECIAL_unknown) Am I
> missing something here (from decode_cmdline_options_to_array). I
> expected it to be OPT_Xassembler
Hmm, not sure what's going on there, you'd have to trace through the
opts-common.c code.
> >
> > 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.
> in
> +
> +static void
> +append_compiler_wa_options (obstack *argv_obstack,
> + struct cl_decoded_option *opts,
> + unsigned int count)
> +{
> + static const char *collect_as = NULL;
> + for (unsigned int j = 1; j < count; ++j)
> + {
> + struct cl_decoded_option *option = &opts[j];
> + if (option->opt_index != OPT_Wa_)
> + continue;
> + const char *args_text = option->orig_option_with_args_text;
> + /* We expect all the -Wa, options to be same. */
> + if (collect_as && strcmp (collect_as, args_text) != 0)
> + fatal_error (input_location, "-Wa, options does not match");
> + if (!collect_as)
> + {
> + obstack_ptr_grow (argv_obstack, args_text);
> + collect_as = args_text;
> + }
> + }
> +}
> I am only copying once. Also accept if all the arguments are same or
> empty. Am I missing something here.
Uh, OK. I didn't expect the code to be in append_compiler_wa_options
and I was refering to the OPT_Wa_ handling that appends it each time
we see it.
> >
> > Your append_compiler_wa_options should be merged into
> > append_compiler_options, passing -Xassembler through.
>
> OK.
>
> >
> > Thanks,
> > Richard.
> >
> > > Thanks,
> > > Kugan
> > >
> > > >
> > > >
> > > > H.J.
