diff mbox series

driver: Also prune joined switches with negation

Message ID 20190924040427.32342-1-mattst88@gmail.com
State New
Headers show
Series driver: Also prune joined switches with negation | expand

Commit Message

Matt Turner Sept. 24, 2019, 4:04 a.m. UTC
When -march=native is passed to host_detect_local_cpu to the backend,
it overrides all command lines after it.  That means

$ gcc -march=native -march=armv8-a

is treated as

$ gcc -march=armv8-a -march=native

Prune joined switches with Negative and RejectNegative to allow
-march=armv8-a to override previous -march=native on command-line.

This is the same fix as was applied for i386 in SVN revision 269164 but for
aarch64 and arm.

gcc/

	PR driver/69471
	* config/aarch64/aarch64.opt (march=): Add Negative(march=).
	(mtune=): Add Negative(mtune=).
	* config/arm/arm.opt: Likewise.
---
 gcc/config/aarch64/aarch64.opt | 5 +++--
 gcc/config/arm/arm.opt         | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Kyrill Tkachov Sept. 24, 2019, 8:24 a.m. UTC | #1
Hi Matt,

On 9/24/19 5:04 AM, Matt Turner wrote:
> When -march=native is passed to host_detect_local_cpu to the backend,
> it overrides all command lines after it.  That means
>
> $ gcc -march=native -march=armv8-a
>
> is treated as
>
> $ gcc -march=armv8-a -march=native
>
> Prune joined switches with Negative and RejectNegative to allow
> -march=armv8-a to override previous -march=native on command-line.
>
> This is the same fix as was applied for i386 in SVN revision 269164 
> but for
> aarch64 and arm.
>
The fix is ok for arm and LGTM for aarch64 FWIW.

How has this been tested?

However...


> gcc/
>
>         PR driver/69471
>         * config/aarch64/aarch64.opt (march=): Add Negative(march=).
>         (mtune=): Add Negative(mtune=).
>         * config/arm/arm.opt: Likewise.
> ---
>  gcc/config/aarch64/aarch64.opt | 5 +++--
>  gcc/config/arm/arm.opt         | 4 ++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.opt 
> b/gcc/config/aarch64/aarch64.opt
> index 865b6a6d8ca..908dca23b3c 100644
> --- a/gcc/config/aarch64/aarch64.opt
> +++ b/gcc/config/aarch64/aarch64.opt
> @@ -119,7 +119,8 @@ EnumValue
>  Enum(aarch64_tls_size) String(48) Value(48)
>
>  march=
> -Target RejectNegative ToLower Joined Var(aarch64_arch_string)
> +Target RejectNegative Negative(march=) ToLower Joined 
> Var(aarch64_arch_string)
> +
>  Use features of architecture ARCH.
>
>  mcpu=


... Looks like we'll need something similar for -mcpu. On arm and 
aarch64 the -mcpu is the most commonly used option and that can also 
take a "native" value that would suffer from the same issue I presume.

Thanks,

Kyrill


> @@ -127,7 +128,7 @@ Target RejectNegative ToLower Joined 
> Var(aarch64_cpu_string)
>  Use features of and optimize for CPU.
>
>  mtune=
> -Target RejectNegative ToLower Joined Var(aarch64_tune_string)
> +Target RejectNegative Negative(mtune=) ToLower Joined 
> Var(aarch64_tune_string)
>  Optimize for CPU.
>
>  mabi=
> diff --git a/gcc/config/arm/arm.opt b/gcc/config/arm/arm.opt
> index 452f0cf6d67..e3ead5c95d1 100644
> --- a/gcc/config/arm/arm.opt
> +++ b/gcc/config/arm/arm.opt
> @@ -82,7 +82,7 @@ mapcs-stack-check
>  Target Report Mask(APCS_STACK) Undocumented
>
>  march=
> -Target RejectNegative ToLower Joined Var(arm_arch_string)
> +Target RejectNegative Negative(march=) ToLower Joined 
> Var(arm_arch_string)
>  Specify the name of the target architecture.
>
>  ; Other arm_arch values are loaded from arm-tables.opt
> @@ -232,7 +232,7 @@ Target Report Mask(TPCS_LEAF_FRAME)
>  Thumb: Generate (leaf) stack frames even if not needed.
>
>  mtune=
> -Target RejectNegative ToLower Joined Var(arm_tune_string)
> +Target RejectNegative Negative(mtune=) ToLower Joined 
> Var(arm_tune_string)
>  Tune code for the given processor.
>
>  mprint-tune-info
> -- 
> 2.21.0
>
Matt Turner Sept. 24, 2019, 6:57 p.m. UTC | #2
On Tue, Sep 24, 2019 at 1:24 AM Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
> Hi Matt,
>
> On 9/24/19 5:04 AM, Matt Turner wrote:
> > When -march=native is passed to host_detect_local_cpu to the backend,
> > it overrides all command lines after it.  That means
> >
> > $ gcc -march=native -march=armv8-a
> >
> > is treated as
> >
> > $ gcc -march=armv8-a -march=native
> >
> > Prune joined switches with Negative and RejectNegative to allow
> > -march=armv8-a to override previous -march=native on command-line.
> >
> > This is the same fix as was applied for i386 in SVN revision 269164
> > but for
> > aarch64 and arm.
> >
> The fix is ok for arm and LGTM for aarch64 FWIW.

Thanks!

> How has this been tested?

The problem was noticed in this bug report:

   https://bugs.gentoo.org/693522

I remembered seeing the i386 fix and I separately encountered the
problem on ARM when building the pixman library which has iwMMXt code
which requires march=iwmmxt (Could I bribe someone into fixing that by
giving gcc an -miwmmxt flag?)

I verified the fix works by patching gcc and seeing that nss (the
package from the Gentoo bug report) successfully builds with
CFLAGS="-march=native -O2 -pipe"

SVN revision 269164 also added some tests to the gcc test suite, but I
am not sufficiently familiar with building gcc and running the test
suite to verify that any test I speculatively add actually works.

> However...
>
>
> > gcc/
> >
> >         PR driver/69471
> >         * config/aarch64/aarch64.opt (march=): Add Negative(march=).
> >         (mtune=): Add Negative(mtune=).
> >         * config/arm/arm.opt: Likewise.
> > ---
> >  gcc/config/aarch64/aarch64.opt | 5 +++--
> >  gcc/config/arm/arm.opt         | 4 ++--
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/gcc/config/aarch64/aarch64.opt
> > b/gcc/config/aarch64/aarch64.opt
> > index 865b6a6d8ca..908dca23b3c 100644
> > --- a/gcc/config/aarch64/aarch64.opt
> > +++ b/gcc/config/aarch64/aarch64.opt
> > @@ -119,7 +119,8 @@ EnumValue
> >  Enum(aarch64_tls_size) String(48) Value(48)
> >
> >  march=
> > -Target RejectNegative ToLower Joined Var(aarch64_arch_string)
> > +Target RejectNegative Negative(march=) ToLower Joined
> > Var(aarch64_arch_string)
> > +
> >  Use features of architecture ARCH.
> >
> >  mcpu=
>
>
> ... Looks like we'll need something similar for -mcpu. On arm and
> aarch64 the -mcpu is the most commonly used option and that can also
> take a "native" value that would suffer from the same issue I presume.

Thank you. I've sent a second version with this addressed in reply to
my initial patch.

If the patch is okay, I think we'd appreciate it if it were backported
to the gcc-8 branch as well.
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt
index 865b6a6d8ca..908dca23b3c 100644
--- a/gcc/config/aarch64/aarch64.opt
+++ b/gcc/config/aarch64/aarch64.opt
@@ -119,7 +119,8 @@  EnumValue
 Enum(aarch64_tls_size) String(48) Value(48)
 
 march=
-Target RejectNegative ToLower Joined Var(aarch64_arch_string)
+Target RejectNegative Negative(march=) ToLower Joined Var(aarch64_arch_string)
+
 Use features of architecture ARCH.
 
 mcpu=
@@ -127,7 +128,7 @@  Target RejectNegative ToLower Joined Var(aarch64_cpu_string)
 Use features of and optimize for CPU.
 
 mtune=
-Target RejectNegative ToLower Joined Var(aarch64_tune_string)
+Target RejectNegative Negative(mtune=) ToLower Joined Var(aarch64_tune_string)
 Optimize for CPU.
 
 mabi=
diff --git a/gcc/config/arm/arm.opt b/gcc/config/arm/arm.opt
index 452f0cf6d67..e3ead5c95d1 100644
--- a/gcc/config/arm/arm.opt
+++ b/gcc/config/arm/arm.opt
@@ -82,7 +82,7 @@  mapcs-stack-check
 Target Report Mask(APCS_STACK) Undocumented
 
 march=
-Target RejectNegative ToLower Joined Var(arm_arch_string)
+Target RejectNegative Negative(march=) ToLower Joined Var(arm_arch_string)
 Specify the name of the target architecture.
 
 ; Other arm_arch values are loaded from arm-tables.opt
@@ -232,7 +232,7 @@  Target Report Mask(TPCS_LEAF_FRAME)
 Thumb: Generate (leaf) stack frames even if not needed.
 
 mtune=
-Target RejectNegative ToLower Joined Var(arm_tune_string)
+Target RejectNegative Negative(mtune=) ToLower Joined Var(arm_tune_string)
 Tune code for the given processor.
 
 mprint-tune-info