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.
Thanks,
Kugan
>
> > I therefore propose that we take the simpler approach first and
>
> It works for me.
>
> > improve it by modifying the LTO partitioning. Any thoughts?
> >
>
> Thanks.
>
> --
> H.J.
From 1ff490834f4a428da631d17b155074be17c4dfd7 Mon Sep 17 00:00:00 2001
From: Kugan <kugan.vivekanandarajah@linaro.org>
Date: Sat, 28 Sep 2019 02:11:49 +1000
Subject: [PATCH] COLLECT_AS support
Change-Id: I5b163d9681d74e2a348c09fc7ed6f0ef36a4b4a6
---
gcc/gcc.c | 29 ++++++++++++++++++++
gcc/lto-opts.c | 16 +++++++++--
gcc/lto-wrapper.c | 35 ++++++++++++++++++++++++
gcc/testsuite/gcc.target/arm/pr78353-1.c | 9 ++++++
gcc/testsuite/gcc.target/arm/pr78353-2.c | 9 ++++++
5 files changed, 96 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/arm/pr78353-1.c
create mode 100644 gcc/testsuite/gcc.target/arm/pr78353-2.c
diff --git a/gcc/gcc.c b/gcc/gcc.c
index 1216cdd505a..3336160e3e2 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -5239,6 +5239,34 @@ do_specs_vec (vec<char_p> vec)
}
}
+/* 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=-Wa,",
+ sizeof ("COLLECT_AS_OPTIONS=-Wa,") - 1);
+
+ FOR_EACH_VEC_ELT (vec, ix, opt)
+ {
+ obstack_grow (&collect_obstack, opt, strlen (opt));
+ --len;
+ if (len)
+ obstack_grow (&collect_obstack, ",", strlen (","));
+ }
+
+ obstack_1grow (&collect_obstack, '\0');
+ xputenv (XOBFINISH (&collect_obstack, char *));
+}
+
/* Process the sub-spec SPEC as a portion of a larger spec.
This is like processing a whole spec except that we do
not initialize at the beginning and we do not supply a
@@ -7360,6 +7388,7 @@ driver::main (int argc, char **argv)
global_initializations ();
build_multilib_strings ();
set_up_specs ();
+ putenv_COLLECT_AS_OPTIONS (assembler_options);
putenv_COLLECT_GCC (argv[0]);
maybe_putenv_COLLECT_LTO_WRAPPER ();
maybe_putenv_OFFLOAD_TARGETS ();
diff --git a/gcc/lto-opts.c b/gcc/lto-opts.c
index 0e9f24e1189..d89fe5cc4f9 100644
--- a/gcc/lto-opts.c
+++ b/gcc/lto-opts.c
@@ -166,8 +166,20 @@ lto_write_options (void)
obstack_grow (&temporary_obstack, "\0", 1);
args = XOBFINISH (&temporary_obstack, char *);
lto_write_data (args, strlen (args) + 1);
- lto_end_section ();
-
obstack_free (&temporary_obstack, NULL);
+ const char *collect_as_options = getenv ("COLLECT_AS_OPTIONS");
+
+ if (collect_as_options)
+ {
+ obstack_init (&temporary_obstack);
+ append_to_collect_gcc_options (&temporary_obstack, &first_p,
+ collect_as_options);
+ obstack_grow (&temporary_obstack, "\0", 1);
+ args = XOBFINISH (&temporary_obstack, char *);
+ lto_write_data (args, strlen (args) + 1);
+ obstack_free (&temporary_obstack, NULL);
+ }
+
+ lto_end_section ();
free (section_name);
}
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;
@@ -574,6 +579,32 @@ parse_env_var (const char *str, char ***pvalues, const char *append)
return num;
}
+/* Append options OPTS from -Wa, options to ARGV_OBSTACK. */
+
+static void
+append_compiler_wa_options (obstack *argv_obstack,
+ struct cl_decoded_option *opts,
+ unsigned int count)
+{
+ 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;
+ 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;
+ }
+ }
+}
/* Append options OPTS from lto or offload_lto sections to ARGV_OBSTACK. */
static void
@@ -846,6 +877,8 @@ compile_offload_image (const char *target, const char *compiler_path,
/* Append options from offload_lto sections. */
append_compiler_options (&argv_obstack, compiler_opts,
compiler_opt_count);
+ append_compiler_wa_options (&argv_obstack, compiler_opts,
+ compiler_opt_count);
append_diag_options (&argv_obstack, linker_opts, linker_opt_count);
/* Append options specified by -foffload last. In case of conflicting
@@ -1349,6 +1382,8 @@ run_gcc (unsigned argc, char *argv[])
append_compiler_options (&argv_obstack, fdecoded_options,
fdecoded_options_count);
+ append_compiler_wa_options (&argv_obstack, fdecoded_options,
+ fdecoded_options_count);
append_linker_options (&argv_obstack, decoded_options, decoded_options_count);
/* Scan linker driver arguments for things that are of relevance to us. */
diff --git a/gcc/testsuite/gcc.target/arm/pr78353-1.c b/gcc/testsuite/gcc.target/arm/pr78353-1.c
new file mode 100644
index 00000000000..bba81ee50c3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr78353-1.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-march=armv7-a -mthumb -O2 -flto -Wa,-mimplicit-it=always" } */
+
+int main(int x)
+{
+ asm("teq %0, #0; addne %0, %0, #1" : "=r" (x));
+ return x;
+}
+
diff --git a/gcc/testsuite/gcc.target/arm/pr78353-2.c b/gcc/testsuite/gcc.target/arm/pr78353-2.c
new file mode 100644
index 00000000000..776eb64b8c7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr78353-2.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-march=armv7-a -mthumb -O2 -flto -Wa,-mimplicit-it=always,-mthumb" } */
+
+int main(int x)
+{
+ asm("teq %0, #0; addne %0, %0, #1" : "=r" (x));
+ return x;
+}
+
--
2.17.1
