rs6000: Don't ICE when generating vector pair load/store insns [PR110411]

  Hi All,

The following patch has been bootstrapped and regtested on powerpc64le-linux.

while generating vector pairs of load & store instruction, the src address
was treated as an altivec type and that type of address is invalid for 
lxvp and stxvp insns. The solution for this is to avoid altivec type address
for OOmode and XOmode.

2023-07-05  Jeevitha Palanisamy  <jeevitha@linux.ibm.com>

gcc/
	PR target/110411
	* config/rs6000/rs6000.cc (rs6000_legitimate_address_p): Avoid altivec
	address for OOmode and XOmde.

gcc/testsuite/
	PR target/110411
	* gcc.target/powerpc/pr110411.c: New testcase.
  

Hi!

On Wed, Jul 05, 2023 at 05:21:18PM +0530, P Jeevitha wrote:
> The following patch has been bootstrapped and regtested on powerpc64le-linux.
> 
> while generating vector pairs of load & store instruction, the src address
> was treated as an altivec type and that type of address is invalid for 
> lxvp and stxvp insns. The solution for this is to avoid altivec type address
> for OOmode and XOmode.

The mail message you send should be what will end up in the Git commit
message.  Your lines are too long for that (and the subject is much too
long btw), and the content isn't right either.

Maybe something like

"""
rs6000: Don't allow OOmode or XOmode in AltiVec addresses (PR110411)

There are no instructions that do traditional AltiVec addresses (i.e.
with the low four bits of the address masked off) for OOmode and XOmode
objects.  Don't allow those in rs6000_legitimate_address_p.
"""

> gcc/
> 	PR target/110411
> 	* config/rs6000/rs6000.cc (rs6000_legitimate_address_p): Avoid altivec
> 	address for OOmode and XOmde.

(XOmode, sp.)

Not "avoid", disallow.  If you avoid something you still allow it, you
just prefer to see something else.

> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -9894,6 +9894,8 @@ rs6000_legitimate_address_p (machine_mode mode, rtx x, bool reg_ok_strict)
>  
>    /* Handle unaligned altivec lvx/stvx type addresses.  */
>    if (VECTOR_MEM_ALTIVEC_OR_VSX_P (mode)
> +      && mode !=  OOmode
> +      && mode !=  XOmode
>        && GET_CODE (x) == AND
>        && CONST_INT_P (XEXP (x, 1))
>        && INTVAL (XEXP (x, 1)) == -16)

Why do we need this for OOmode and XOmode here, but not for the other
modes that are equally not allowed?  That makes no sense.

Should you check for anything that is more than a register, for example?
If so, do *that*?

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr110411.c
> @@ -0,0 +1,21 @@
> +/* PR target/110411 */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -S -mblock-ops-vector-pair" } */

-S in testcases is wrong.  Why do you want this?  It is *good* if this
is hauled through the assembler as well!  If you *really* want this you
use "dg-do assemble", but you shouldn't.


Segher
  

On 7/6/23 12:33 PM, Segher Boessenkool wrote:
> On Wed, Jul 05, 2023 at 05:21:18PM +0530, P Jeevitha wrote:
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -9894,6 +9894,8 @@ rs6000_legitimate_address_p (machine_mode mode, rtx x, bool reg_ok_strict)
>>  
>>    /* Handle unaligned altivec lvx/stvx type addresses.  */
>>    if (VECTOR_MEM_ALTIVEC_OR_VSX_P (mode)
>> +      && mode !=  OOmode
>> +      && mode !=  XOmode
>>        && GET_CODE (x) == AND
>>        && CONST_INT_P (XEXP (x, 1))
>>        && INTVAL (XEXP (x, 1)) == -16)
> 
> Why do we need this for OOmode and XOmode here, but not for the other
> modes that are equally not allowed?  That makes no sense.

VECTOR_MEM_ALTIVEC_OR_VSX_P (mode) already filters those modes out
(eg, SImode, DFmode, etc.), just not OOmode and XOmode, since those both
are modes used in/with VSX registers.



> Should you check for anything that is more than a register, for example?
> If so, do *that*?

Well rs6000_legitimate_address_p() is only passed the MEM rtx, so we have
no idea if this is a load or store, so we're clueless on number of regs
needed to hold this mode.  The best we could do is something like

  GET_MODE_SIZE (mode) == GET_MODE_SIZE (V16QImode)

or some such thing.  Would you prefer something like that?



>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr110411.c
>> @@ -0,0 +1,21 @@
>> +/* PR target/110411 */
>> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -S -mblock-ops-vector-pair" } */
> 
> -S in testcases is wrong.  Why do you want this?  It is *good* if this
> is hauled through the assembler as well!  If you *really* want this you
> use "dg-do assemble", but you shouldn't.

For test cases checking for ICEs, we don't need to assemble, so I agree,
we just need to remove the -S option, which is implied by this being a
dg-do compile test case (the default for this test directory).


Peter
  

On Thu, Jul 06, 2023 at 02:48:19PM -0500, Peter Bergner wrote:
> On 7/6/23 12:33 PM, Segher Boessenkool wrote:
> > On Wed, Jul 05, 2023 at 05:21:18PM +0530, P Jeevitha wrote:
> >> --- a/gcc/config/rs6000/rs6000.cc
> >> +++ b/gcc/config/rs6000/rs6000.cc
> >> @@ -9894,6 +9894,8 @@ rs6000_legitimate_address_p (machine_mode mode, rtx x, bool reg_ok_strict)
> >>  
> >>    /* Handle unaligned altivec lvx/stvx type addresses.  */
> >>    if (VECTOR_MEM_ALTIVEC_OR_VSX_P (mode)
> >> +      && mode !=  OOmode
> >> +      && mode !=  XOmode
> >>        && GET_CODE (x) == AND
> >>        && CONST_INT_P (XEXP (x, 1))
> >>        && INTVAL (XEXP (x, 1)) == -16)
> > 
> > Why do we need this for OOmode and XOmode here, but not for the other
> > modes that are equally not allowed?  That makes no sense.
> 
> VECTOR_MEM_ALTIVEC_OR_VSX_P (mode) already filters those modes out
> (eg, SImode, DFmode, etc.), just not OOmode and XOmode, since those both
> are modes used in/with VSX registers.

It does not filter anything out, no.  That simply checks if a datum of
that mode can be loaded into vector registers or not.  For example
SImode could very well be loaded into vector registers!  (It just is not
such a great idea).

And for some reason there is VECTOR_P8_VECTOR as well, which is mixing
multiple concepts already.  Let's not add more, _please_.

> > Should you check for anything that is more than a register, for example?
> > If so, do *that*?
> 
> Well rs6000_legitimate_address_p() is only passed the MEM rtx, so we have
> no idea if this is a load or store, so we're clueless on number of regs
> needed to hold this mode.  The best we could do is something like

That is *bigger than* a register.  It's the same in Dutch, sorry, I am
tired :-(

>   GET_MODE_SIZE (mode) == GET_MODE_SIZE (V16QImode)
> 
> or some such thing.  Would you prefer something like that?

That is even worse :-(

> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/powerpc/pr110411.c
> >> @@ -0,0 +1,21 @@
> >> +/* PR target/110411 */
> >> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -S -mblock-ops-vector-pair" } */
> > 
> > -S in testcases is wrong.  Why do you want this?  It is *good* if this
> > is hauled through the assembler as well!  If you *really* want this you
> > use "dg-do assemble", but you shouldn't.
> 
> For test cases checking for ICEs, we don't need to assemble, so I agree,
> we just need to remove the -S option, which is implied by this being a
> dg-do compile test case (the default for this test directory).

We *do* want to assemble.  It is a general principle that we want to
test as much as possible whenever possible.  *Most* problems are found
with the help of testcases that were never designed for the problem
found!

dg-do compile *does* invoke the assembler, btw.  As it should.


Segher
  

On 7/6/23 6:28 PM, Segher Boessenkool wrote:
> On Thu, Jul 06, 2023 at 02:48:19PM -0500, Peter Bergner wrote:
>> On 7/6/23 12:33 PM, Segher Boessenkool wrote:
>>> On Wed, Jul 05, 2023 at 05:21:18PM +0530, P Jeevitha wrote:
>>>> --- a/gcc/config/rs6000/rs6000.cc
>>>> +++ b/gcc/config/rs6000/rs6000.cc
>>>> @@ -9894,6 +9894,8 @@ rs6000_legitimate_address_p (machine_mode mode, rtx x, bool reg_ok_strict)
>>>>  
>>>>    /* Handle unaligned altivec lvx/stvx type addresses.  */
>>>>    if (VECTOR_MEM_ALTIVEC_OR_VSX_P (mode)
>>>> +      && mode !=  OOmode
>>>> +      && mode !=  XOmode
>>>>        && GET_CODE (x) == AND
>>>>        && CONST_INT_P (XEXP (x, 1))
>>>>        && INTVAL (XEXP (x, 1)) == -16)
>>>
>>> Why do we need this for OOmode and XOmode here, but not for the other
>>> modes that are equally not allowed?  That makes no sense.
>>
>> VECTOR_MEM_ALTIVEC_OR_VSX_P (mode) already filters those modes out
>> (eg, SImode, DFmode, etc.), just not OOmode and XOmode, since those both
>> are modes used in/with VSX registers.
> 
> It does not filter anything out, no.  That simply checks if a datum of
> that mode can be loaded into vector registers or not.  For example
> SImode could very well be loaded into vector registers!  (It just is not
> such a great idea).

I spent some time looking at how the compiler fixes this up in the
-mno-block-ops-vector-pair case and I see the constraints used in the
vsx_mov<mode>_64bit pattern for loads and stores disallows these types
of addresses, so LRA fixes them up for us.  Clearly movoo should do the
same and that is enough to fix the ICE.  I'll work with Jeevitha on
submitting a patch using that solution.

That said, I think it would be good to modify rs6000_legitimate_address_p
to disallow these altivec style addresses for OOmode and XOmode, since we
know early-on that they're not going to be valid, but that would be a
different patch.




> dg-do compile *does* invoke the assembler, btw.  As it should.

There is dg-do "preprocess", "compile", "assemble", "link" and "run"
(ignoring "precompile" and "repo").  Dg-do compile produces an assembly
file, but doesn't actually call the assembler, which we don't strictly
need for a test case that checks whether GCC ICEs or not.  If you want
to run the assembler too and then stop, then you'd want dg-do assemble.

Peter