[rs6000] Fix PR target/63177: Powerpc no-vfa-vect-depend-2.c and no-vfa-vect-depend-3.c failures

Message ID d9a22948-f2c0-c420-67d4-4e667ce1e521@vnet.ibm.com
State New
Headers show
Series
  • [rs6000] Fix PR target/63177: Powerpc no-vfa-vect-depend-2.c and no-vfa-vect-depend-3.c failures
Related show

Commit Message

Peter Bergner June 5, 2018, 1:57 a.m.
PR63177 shows a bug in how we determine which gas options we decide to pass to the
assembler.  Normally, we pass the -m<CPU> option to the assembler if we used the
-mcpu=<CPU> option.  However, if we don't compile with -mcpu=<CPU>, then we will
check some of the -m<vector option> options and pass an appropriate -m<CPU> option
to the assembler.  This is all fine and good except for when we compile with
-mpower9-vector -mcpu=power8.  The problem here is that POWER9 added new lxvx/stxvx
instructions which already existed in POWER8 as extended mnemonics of lxvd2x/stxvd2x
which are different instructions and behave differently in LE mode.  The "bug" is
that -mpower9-vector enables the generation of the POWER9 lxvx instruction, but the
-mcpu=power8 option causes us to use the -mpower8 assembler option so we get the
wrong instruction. :-(

The fix used here is to catch the special case when we use -mpower9-vector and
-mcpu=power8 together and then force ourselves to use the -mpower9 gas option.

This passed bootstrap and regtesting with no regressions and fixes a little over
150 testsuite failures.  Ok for trunk and the appropriate release branches once
it's baked on trunk for a while?

Peter

	PR target/63177
	* /config/rs6000/rs6000.h (ASM_CPU_SPEC): Add support for -mpower9.
	Don't handle -mcpu=power8 if -mpower9-vector is also used.

Comments

Segher Boessenkool June 5, 2018, 4:23 p.m. | #1
On Mon, Jun 04, 2018 at 08:57:24PM -0500, Peter Bergner wrote:
> PR63177 shows a bug in how we determine which gas options we decide to pass to the

> assembler.  Normally, we pass the -m<CPU> option to the assembler if we used the

> -mcpu=<CPU> option.  However, if we don't compile with -mcpu=<CPU>, then we will

> check some of the -m<vector option> options and pass an appropriate -m<CPU> option

> to the assembler.  This is all fine and good except for when we compile with

> -mpower9-vector -mcpu=power8.  The problem here is that POWER9 added new lxvx/stxvx

> instructions which already existed in POWER8 as extended mnemonics of lxvd2x/stxvd2x

> which are different instructions and behave differently in LE mode.  The "bug" is

> that -mpower9-vector enables the generation of the POWER9 lxvx instruction, but the

> -mcpu=power8 option causes us to use the -mpower8 assembler option so we get the

> wrong instruction. :-(

> 

> The fix used here is to catch the special case when we use -mpower9-vector and

> -mcpu=power8 together and then force ourselves to use the -mpower9 gas option.


Ideally -mpowerN-vector will just go away.

> --- gcc/config/rs6000/rs6000.h	(revision 260913)

> +++ gcc/config/rs6000/rs6000.h	(working copy)

> @@ -120,7 +120,7 @@

>  %{mcpu=power6: %(asm_cpu_power6) -maltivec} \

>  %{mcpu=power6x: %(asm_cpu_power6) -maltivec} \

>  %{mcpu=power7: %(asm_cpu_power7)} \

> -%{mcpu=power8: %(asm_cpu_power8)} \

> +%{mcpu=power8: %{!mpower9-vector: %(asm_cpu_power8)}} \

>  %{mcpu=power9: %(asm_cpu_power9)} \

>  %{mcpu=a2: -ma2} \

>  %{mcpu=powerpc: -mppc} \

> @@ -169,6 +169,7 @@

>  %{maltivec: -maltivec} \

>  %{mvsx: -mvsx %{!maltivec: -maltivec} %{!mcpu*: %(asm_cpu_power7)}} \

>  %{mpower8-vector|mcrypto|mdirect-move|mhtm: %{!mcpu*: %(asm_cpu_power8)}} \

> +%{mpower9-vector: %{!mcpu*|mcpu=power8: %(asm_cpu_power9)}} \

>  -many"


Why do you need the !mpower9-vector in the mcpu=power8 clause?  Is how
mpower8-vector is handled not correct, or is something fundamentally
different there?


Segher
Peter Bergner June 5, 2018, 5:26 p.m. | #2
On 6/5/18 11:23 AM, Segher Boessenkool wrote:
> On Mon, Jun 04, 2018 at 08:57:24PM -0500, Peter Bergner wrote:

>> The fix used here is to catch the special case when we use -mpower9-vector and

>> -mcpu=power8 together and then force ourselves to use the -mpower9 gas option.

> 

> Ideally -mpowerN-vector will just go away.


No argument from me.



>> -%{mcpu=power8: %(asm_cpu_power8)} \

>> +%{mcpu=power8: %{!mpower9-vector: %(asm_cpu_power8)}} \

>>  %{mcpu=power9: %(asm_cpu_power9)} \

>>  %{mcpu=a2: -ma2} \

>>  %{mcpu=powerpc: -mppc} \

>> @@ -169,6 +169,7 @@

>>  %{maltivec: -maltivec} \

>>  %{mvsx: -mvsx %{!maltivec: -maltivec} %{!mcpu*: %(asm_cpu_power7)}} \

>>  %{mpower8-vector|mcrypto|mdirect-move|mhtm: %{!mcpu*: %(asm_cpu_power8)}} \

>> +%{mpower9-vector: %{!mcpu*|mcpu=power8: %(asm_cpu_power9)}} \

>>  -many"

> 

> Why do you need the !mpower9-vector in the mcpu=power8 clause?  Is how

> mpower8-vector is handled not correct, or is something fundamentally

> different there?


It's fundamentally different because of the overlapping lxvx/stxvx mnemonics
between P8 and P9, which doesn't occur between any other ISA levels.
Similar to gcc handling of options, gas uses the last -m<CPU> option to
assemble with, so if one were to pass -mpower9 -mpower8 to the assembler
(which you would get if you compile with -mpower9-vector -mcpu=power8),
then we'd assemble for power8 and get the P8's lxvx extended mnemonic.
So I've done that to "fix" any ordering issues.

Initially, I was thinking that we should determine what -m<CPU> gas option
to use by looking at the rs6000_isa_flags value to compute given all of the
gcc -m* and -mcpu=* options, but unfortunately, the gas option is computed
in the gcc driver and we don't have access to rs6000_isa_flags.  I thought
the fix above was the least of the bad solutions. :-)

Peter
Segher Boessenkool June 5, 2018, 8:22 p.m. | #3
On Tue, Jun 05, 2018 at 12:26:04PM -0500, Peter Bergner wrote:
> >> -%{mcpu=power8: %(asm_cpu_power8)} \

> >> +%{mcpu=power8: %{!mpower9-vector: %(asm_cpu_power8)}} \

> >>  %{mcpu=power9: %(asm_cpu_power9)} \

> >>  %{mcpu=a2: -ma2} \

> >>  %{mcpu=powerpc: -mppc} \

> >> @@ -169,6 +169,7 @@

> >>  %{maltivec: -maltivec} \

> >>  %{mvsx: -mvsx %{!maltivec: -maltivec} %{!mcpu*: %(asm_cpu_power7)}} \

> >>  %{mpower8-vector|mcrypto|mdirect-move|mhtm: %{!mcpu*: %(asm_cpu_power8)}} \

> >> +%{mpower9-vector: %{!mcpu*|mcpu=power8: %(asm_cpu_power9)}} \

> >>  -many"

> > 

> > Why do you need the !mpower9-vector in the mcpu=power8 clause?  Is how

> > mpower8-vector is handled not correct, or is something fundamentally

> > different there?

> 

> It's fundamentally different because of the overlapping lxvx/stxvx mnemonics

> between P8 and P9, which doesn't occur between any other ISA levels.

> Similar to gcc handling of options, gas uses the last -m<CPU> option to

> assemble with, so if one were to pass -mpower9 -mpower8 to the assembler

> (which you would get if you compile with -mpower9-vector -mcpu=power8),

> then we'd assemble for power8 and get the P8's lxvx extended mnemonic.

> So I've done that to "fix" any ordering issues.


Ah, that's the crux.  Thanks.  Add a comment to the code please?

> Initially, I was thinking that we should determine what -m<CPU> gas option

> to use by looking at the rs6000_isa_flags value to compute given all of the

> gcc -m* and -mcpu=* options, but unfortunately, the gas option is computed

> in the gcc driver and we don't have access to rs6000_isa_flags.  I thought

> the fix above was the least of the bad solutions. :-)


It isn't nice, but it looks like it will work.

Okay for trunk.  Thanks,


Segher
Peter Bergner June 6, 2018, 3:03 a.m. | #4
On 6/5/18 3:22 PM, Segher Boessenkool wrote:
> Ah, that's the crux.  Thanks.  Add a comment to the code please?


Like so?  I tried placing it next to the mcpu=power8 clause, but it seems
you cannot place comments there, as it creates syntax errors during the build.

 /* Common ASM definitions used by ASM_SPEC among the various targets for
    handling -mcpu=xxx switches.  There is a parallel list in driver-rs6000.c to
    provide the default assembler options if the user uses -mcpu=native, so if
-   you make changes here, make them also there.  */
+   you make changes here, make them also there.  PR63177: Do not pass -mpower8
+   to the assembler if -mpower9-vector was also used.  */
 #define ASM_CPU_SPEC \
...


Peter
Segher Boessenkool June 6, 2018, 7:54 p.m. | #5
On Tue, Jun 05, 2018 at 10:03:46PM -0500, Peter Bergner wrote:
> On 6/5/18 3:22 PM, Segher Boessenkool wrote:

> > Ah, that's the crux.  Thanks.  Add a comment to the code please?

> 

> Like so?  I tried placing it next to the mcpu=power8 clause, but it seems

> you cannot place comments there, as it creates syntax errors during the build.

> 

>  /* Common ASM definitions used by ASM_SPEC among the various targets for

>     handling -mcpu=xxx switches.  There is a parallel list in driver-rs6000.c to

>     provide the default assembler options if the user uses -mcpu=native, so if

> -   you make changes here, make them also there.  */

> +   you make changes here, make them also there.  PR63177: Do not pass -mpower8

> +   to the assembler if -mpower9-vector was also used.  */

>  #define ASM_CPU_SPEC \

> ...


Yes that's great, thanks!  Backports are okay, too.


Segher
Peter Bergner June 7, 2018, 2:06 p.m. | #6
On 6/6/18 2:54 PM, Segher Boessenkool wrote:
> On Tue, Jun 05, 2018 at 10:03:46PM -0500, Peter Bergner wrote:

>> On 6/5/18 3:22 PM, Segher Boessenkool wrote:

>>> Ah, that's the crux.  Thanks.  Add a comment to the code please?

>>

>> Like so?  I tried placing it next to the mcpu=power8 clause, but it seems

>> you cannot place comments there, as it creates syntax errors during the build.

>>

>>  /* Common ASM definitions used by ASM_SPEC among the various targets for

>>     handling -mcpu=xxx switches.  There is a parallel list in driver-rs6000.c to

>>     provide the default assembler options if the user uses -mcpu=native, so if

>> -   you make changes here, make them also there.  */

>> +   you make changes here, make them also there.  PR63177: Do not pass -mpower8

>> +   to the assembler if -mpower9-vector was also used.  */

>>  #define ASM_CPU_SPEC \

>> ...

> 

> Yes that's great, thanks!  Backports are okay, too.


Looking through the release branches, I had to backport this all the way
back to GCC 6.  Testing on all the release branches were clean so I have
committed everywhere.  Thanks!

Peter

Patch

Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 260913)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -120,7 +120,7 @@ 
 %{mcpu=power6: %(asm_cpu_power6) -maltivec} \
 %{mcpu=power6x: %(asm_cpu_power6) -maltivec} \
 %{mcpu=power7: %(asm_cpu_power7)} \
-%{mcpu=power8: %(asm_cpu_power8)} \
+%{mcpu=power8: %{!mpower9-vector: %(asm_cpu_power8)}} \
 %{mcpu=power9: %(asm_cpu_power9)} \
 %{mcpu=a2: -ma2} \
 %{mcpu=powerpc: -mppc} \
@@ -169,6 +169,7 @@ 
 %{maltivec: -maltivec} \
 %{mvsx: -mvsx %{!maltivec: -maltivec} %{!mcpu*: %(asm_cpu_power7)}} \
 %{mpower8-vector|mcrypto|mdirect-move|mhtm: %{!mcpu*: %(asm_cpu_power8)}} \
+%{mpower9-vector: %{!mcpu*|mcpu=power8: %(asm_cpu_power9)}} \
 -many"
 
 #define CPP_DEFAULT_SPEC ""