[PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Peter Bergner]

The following patch fixes a bug where we do not disable POWER9 vector dform
addressing when we compile for POWER9 but without VSX support.  This manifested
itself with us trying to use dform addressing with altivec loads/stores
which is illegal, leading to an ICE.

This has bootstrapped and regtested with no regessions.  Ok for trunk?

This also affects the FSF 6 branch, ok there too, assuming bootstrap and
regtesting complete cleanly?

Peter

gcc/
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disable
	-mpower9-dform-vector when disabling -mpower9-vector.

gcc/testsuite/
	* gcc.target/powerpc/pr71733.c: New test.


Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 237945)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -4303,7 +4303,8 @@ rs6000_option_override_internal (bool gl
     {
       if (rs6000_isa_flags_explicit & OPTION_MASK_P8_VECTOR)
 	error ("-mpower9-vector requires -mpower8-vector");
-      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
+      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR
+			    | OPTION_MASK_P9_DFORM_VECTOR);
     }
 
   /* There have been bugs with -mvsx-timode that don't show up with -mlra,
Index: gcc/testsuite/gcc.target/powerpc/pr71733.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr71733.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr71733.c	(working copy)
@@ -0,0 +1,14 @@
+/* Test for ICE arising from dform code generation with VSX disabled.  */
+
+/* { dg-do compile } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power9" } } */
+/* { dg-options "-O0 -mcpu=power9 -mno-vsx" } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-skip-if "" { powerpc*-*-aix* } { "*" } { "" } } */
+
+typedef __attribute__((altivec(vector__))) unsigned char vec_t;
+vec_t
+foo (vec_t src)
+{
+  return src;
+}

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[David Edelsohn]

On Tue, Jul 5, 2016 at 10:26 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> The following patch fixes a bug where we do not disable POWER9 vector dform
> addressing when we compile for POWER9 but without VSX support.  This manifested
> itself with us trying to use dform addressing with altivec loads/stores
> which is illegal, leading to an ICE.

Peter,

DFORM definitely should be disabled without VSX, but the patch seems
incomplete.  If VSX and DFORM are enabled, and GCC chooses an Altivec
instruction alternative in a pattern, what is to prevent the
generation of a DFORM address?

Thanks, David

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Peter Bergner]

On 7/6/16 12:53 PM, David Edelsohn wrote:
> On Tue, Jul 5, 2016 at 10:26 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
>> The following patch fixes a bug where we do not disable POWER9 vector dform
>> addressing when we compile for POWER9 but without VSX support.  This manifested
>> itself with us trying to use dform addressing with altivec loads/stores
>> which is illegal, leading to an ICE.
>
> Peter,
>
> DFORM definitely should be disabled without VSX, but the patch seems
> incomplete.  If VSX and DFORM are enabled, and GCC chooses an Altivec
> instruction alternative in a pattern, what is to prevent the
> generation of a DFORM address?

That's a good question.  I'm currently attempting to find out why we
seem to think reg+offset is ok.  With -mcpu=power8 -mno-vsx, we use
reg+reg addressing right from expand.  With -mcpu=power9 -mno-vsx,
we use reg+offset right from expand.  I'll see where the disconnect
is happening.

Peter

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Michael Meissner]

On Tue, Jul 05, 2016 at 09:26:50PM -0500, Peter Bergner wrote:
> The following patch fixes a bug where we do not disable POWER9 vector dform
> addressing when we compile for POWER9 but without VSX support.  This manifested
> itself with us trying to use dform addressing with altivec loads/stores
> which is illegal, leading to an ICE.
> 
> This has bootstrapped and regtested with no regessions.  Ok for trunk?
> 
> This also affects the FSF 6 branch, ok there too, assuming bootstrap and
> regtesting complete cleanly?
> 
> Peter
> 
> gcc/
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Disable
> 	-mpower9-dform-vector when disabling -mpower9-vector.
> 
> gcc/testsuite/
> 	* gcc.target/powerpc/pr71733.c: New test.
> 
> 
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 237945)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -4303,7 +4303,8 @@ rs6000_option_override_internal (bool gl
>      {
>        if (rs6000_isa_flags_explicit & OPTION_MASK_P8_VECTOR)
>  	error ("-mpower9-vector requires -mpower8-vector");
> -      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
> +      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR
> +			    | OPTION_MASK_P9_DFORM_VECTOR);
>      }

Note, this should be

+      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR_SCALAR
+			    | OPTION_MASK_P9_DFORM_VECTOR);

However, we probably need to add all of the other options that depend on VSX.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Peter Bergner]

On 7/6/16 2:19 PM, Michael Meissner wrote:
> On Tue, Jul 05, 2016 at 09:26:50PM -0500, Peter Bergner wrote:
>> -      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
>> +      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR
>> +			    | OPTION_MASK_P9_DFORM_VECTOR);
>>      }
>
> Note, this should be
>
> +      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR_SCALAR
> +			    | OPTION_MASK_P9_DFORM_VECTOR);

I think you mean the following, since we have to disable -mpower9-vector
too:

-      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
+      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR
+			    | OPTION_MASK_P9_DFORM_SCALAR
+			    | OPTION_MASK_P9_DFORM_VECTOR);

I had thought about adding the dform scalar flag, but it was already
correctly disabled and I wasn't sure whether we could have the p9
dform scalar without the vector part.  Probably not, so consider
the patch above as the latest.


> However, we probably need to add all of the other options that depend on VSX.

Yes, there is a cascade affect on the disabling of options when you
explicitly disable something.  It'd be nice if this was somehow
automated, where we have some table showing the dependencies of
the options and the compiler just follows the table disabling things
that depend on something that has been disabled.  Could be hard to
make that dependency list though, given how many we have.

Peter

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Michael Meissner]

On Wed, Jul 06, 2016 at 05:01:38PM -0500, Peter Bergner wrote:
> On 7/6/16 2:19 PM, Michael Meissner wrote:
> >On Tue, Jul 05, 2016 at 09:26:50PM -0500, Peter Bergner wrote:
> >>-      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
> >>+      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR
> >>+			    | OPTION_MASK_P9_DFORM_VECTOR);
> >>     }
> >
> >Note, this should be
> >
> >+      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR_SCALAR
> >+			    | OPTION_MASK_P9_DFORM_VECTOR);
> 
> I think you mean the following, since we have to disable -mpower9-vector
> too:
> 
> -      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
> +      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR
> +			    | OPTION_MASK_P9_DFORM_SCALAR
> +			    | OPTION_MASK_P9_DFORM_VECTOR);
> 
> I had thought about adding the dform scalar flag, but it was already
> correctly disabled and I wasn't sure whether we could have the p9
> dform scalar without the vector part.  Probably not, so consider
> the patch above as the latest.

Yes, you can have P9 dform scalar without P9 dform vector.

> 
> >However, we probably need to add all of the other options that depend on VSX.
> 
> Yes, there is a cascade affect on the disabling of options when you
> explicitly disable something.  It'd be nice if this was somehow
> automated, where we have some table showing the dependencies of
> the options and the compiler just follows the table disabling things
> that depend on something that has been disabled.  Could be hard to
> make that dependency list though, given how many we have.

Yep.  I'm thinking we need masks in rs6000-cpus.def of the options to turn off
if -mno-vsx (and even worse -mno-altivec).

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Peter Bergner]

On 7/6/16 6:29 PM, Michael Meissner wrote:
> On Wed, Jul 06, 2016 at 05:01:38PM -0500, Peter Bergner wrote:
>> I had thought about adding the dform scalar flag, but it was already
>> correctly disabled and I wasn't sure whether we could have the p9
>> dform scalar without the vector part.  Probably not, so consider
>> the patch above as the latest.
>
> Yes, you can have P9 dform scalar without P9 dform vector.

So you're saying my original patch is the correct change?  Ie:

-      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
+      rs6000_isa_flags &= ~(OPTION_MASK_P9_VECTOR
+			    | OPTION_MASK_P9_DFORM_VECTOR);


Peter

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Peter Bergner]

On 7/6/16 12:53 PM, David Edelsohn wrote:
> On Tue, Jul 5, 2016 at 10:26 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
>> The following patch fixes a bug where we do not disable POWER9 vector dform
>> addressing when we compile for POWER9 but without VSX support.  This manifested
>> itself with us trying to use dform addressing with altivec loads/stores
>> which is illegal, leading to an ICE.
> 
> Peter,
> 
> DFORM definitely should be disabled without VSX, but the patch seems
> incomplete.  If VSX and DFORM are enabled, and GCC chooses an Altivec
> instruction alternative in a pattern, what is to prevent the
> generation of a DFORM address?

Adding Uli and Alan to the CC since there are some reload questions. :-)

So I dug into this a little more.  With -mcpu=power9 (ie, no -mno-vsx),
I could not get an altivec pattern generated, without resorting to using
an altivec builtin.  The altivec builtins also seem to force reg+reg
addressing, since hey, that's all they support.  So I couldn't create
a test case to answer your question of what if VSX and DFORM are enabled
and we have an altivec pattern with a dform address.  Maybe there should
be a test case that generates one of the altivec load/store patterns,
but I couldn't do it.

To try and answer your question, I went back to using -mcpu=power9 -mno-vsx
with unpatched trunk (ie, without my patch to disable vector dform when
power9 vector is disabled).  Looking at the following simple load test
case, it seems reload is capable of fixing up dform addresses when they're
used in an altivec pattern:

bergner@genoa:~/gcc/BUGS/sawdey/altivec$ cat vec_load.i
typedef __attribute__((altivec(vector__))) __attribute__((aligned(16))) unsigned char vec_t;
vec_t
foo (vec_t *dst)
{
  return dst[1];
}
bergner@genoa:~/gcc/BUGS/sawdey/altivec$ /home/bergner/gcc/build/gcc-fsf-mainline-debug-3/gcc/xgcc -B/home/bergner/gcc/build/gcc-fsf-mainline-debug-3/gcc -S -O1 -mcpu=power9 -mno-vsx vec_load.i -fdump-rtl-all-all

This gives us the following just before reload:

(insn 11 6 12 2 (set (reg/i:V16QI 79 2)
        (mem:V16QI (plus:DI (reg:DI 3 3 [ dstD.2412 ])
                (const_int 16 [0x10])) [0 MEM[(vec_tD.2411 *)dst_2(D) + 16B]+0 S16 A128])) vec_load.i:6 1209 {*altivec_movv16qi}
     (expr_list:REG_DEAD (reg:DI 3 3 [ dstD.2412 ])
        (nil)))

During reload, we see a reload for input:

Spilling for insn 11.

Reloads for insn # 11
Reload 0: reload_in (DI) = (plus:DI (reg:DI 3 3 [ dstD.2412 ])
                                                    (const_int 16 [0x10]))
	BASE_REGS, RELOAD_FOR_INPUT (opnum = 1), inc by 16
	reload_in_reg: (plus:DI (reg:DI 3 3 [ dstD.2412 ])
                                                    (const_int 16 [0x10]))
	reload_reg_rtx: (reg:DI 3 3)
Insns emitted for these reloads:
(insn 18 16 11 2 (set (reg:DI 3 3)
        (plus:DI (reg:DI 3 3 [ dstD.2412 ])
            (const_int 16 [0x10]))) vec_load.i:6 75 {*adddi3}
     (nil))

...which fixes the address so it satisfies its constraints and leaves us
with the rtl we want:

(insn 18 6 11 2 (set (reg:DI 3 3)
        (plus:DI (reg:DI 3 3 [ dstD.2412 ])
            (const_int 16 [0x10]))) vec_load.i:6 75 {*adddi3}
     (nil))
(insn 11 18 12 2 (set (reg/i:V16QI 79 2)
        (mem:V16QI (reg:DI 3 3) [0 MEM[(vec_tD.2411 *)dst_2(D) + 16B]+0 S16 A128])) vec_load.i:6 1209 {*altivec_movv16qi}
     (nil))

So to answer your question, at least for the altivec load case, reload prevents
us from using reg+offset addressing.


The problem occurs when we have an altivec store, which is what the original
test case had:

bergner@genoa:~/gcc/BUGS/sawdey/altivec$ cat vec_store.i
typedef __attribute__((altivec(vector__))) __attribute__((aligned(16))) unsigned char vec_t;
void
foo (vec_t *dst, vec_t src)
{
  dst[1] = src;
}
bergner@genoa:~/gcc/BUGS/sawdey/altivec$ /home/bergner/gcc/build/gcc-fsf-mainline-debug-3/gcc/xgcc -B/home/bergner/gcc/build/gcc-fsf-mainline-debug-3/gcc -S -O1 -mcpu=power9 -mno-vsx vec_store.i -fdump-rtl-all-all

This gives us the following just before reload:

(insn 7 4 0 2 (set (mem:V16QI (plus:DI (reg:DI 3 3 [ dstD.2412 ])
                (const_int 16 [0x10])) [0 MEM[(vec_tD.2411 *)dst_1(D) + 16B]+0 S16 A128])
        (reg:V16QI 79 2 [ srcD.2413 ])) vec_store.i:5 1209 {*altivec_movv16qi}
     (expr_list:REG_DEAD (reg:V16QI 79 2 [ srcD.2413 ])
        (expr_list:REG_DEAD (reg:DI 3 3 [ dstD.2412 ])
            (nil))))


During reload, we see a reload for output:

Spilling for insn 7.
Using reg 9 for reload 1
Spilling for insn 7.
Using reg 9 for reload 1

Reloads for insn # 7
Reload 0: reload_out (V16QI) = (mem:V16QI (plus:DI (reg:DI 3 3 [ dstD.2412 ])
                                                        (const_int 16 [0x10])) [0 MEM[(vec_tD.2411 *)dst_1(D) + 16B]+0 S16 A128])
        NO_REGS, RELOAD_FOR_OUTPUT (opnum = 0), optional
        reload_out_reg: (mem:V16QI (plus:DI (reg:DI 3 3 [ dstD.2412 ])
                                                        (const_int 16 [0x10])) [0 MEM[(vec_tD.2411 *)dst_1(D) + 16B]+0 S16 A128])
Reload 1: reload_in (V16QI) = (reg:V16QI 79 2 [ srcD.2413 ])
        GENERAL_REGS, RELOAD_FOR_INPUT (opnum = 1)
        reload_in_reg: (reg:V16QI 79 2 [ srcD.2413 ])
        reload_reg_rtx: (reg:V16QI 9 9)
Insns emitted for these reloads:
(insn 12 11 13 2 (set (mem/c:V16QI (plus:DI (reg/f:DI 1 1)
                (const_int -16 [0xfffffffffffffff0])) [0  S16 A128])
        (reg:V16QI 79 2 [ srcD.2413 ])) vec_store.i:5 -1
     (nil))

(insn 13 12 7 2 (set (reg:V16QI 9 9)
        (mem/c:V16QI (plus:DI (reg/f:DI 1 1)
                (const_int -16 [0xfffffffffffffff0])) [0  S16 A128])) vec_store.i:5 -1
     (nil))


The Reload 1 on the other hand is totally bogus.  I'd expect to see an input reload
for the reg+offset address.  Instead, it reloads the src altivec reg and really
creates a mess, since it's loading into a GPR and not a altivec register and
it's using a reg+offset address which is wrong here.

My guess is that we created the input reload for the src reg because
rs6000_legitimate_address_p() and quad_addredss_p() still say that a reg+offset
address for a V16QImode mode is ok, even though in this specific case, it
isn't since we're targeting an altivec store pattern...but they don't know
that.  So reload seems to say, if the address is ok, the src reg must need
reloading.  Did I get that right???

So how do we fix this?  It seems we need a way to disambiguate vector load/store
addresses that should allow reg+offset addresses (when they're targeting one of
the new power9 dform load/store patterns and when they should not allow reg+offset
addresses (when they're targeting older altivec and vsx load/stores that only
support reg+reg addressing).  Does anyone have an idea of how we can accomplish
that?

I'll note that I tried to modify rs6000_secondary_reload_memory to catch
the case where we are passing in an vector dform address with a regclass of
ALTIVEC_REGS, but that didn't seem to help.  That might be because
rs6000_legitimate_address_p() and quad_addredss_p() still say the reg+offset
address are ok.  Do we need to add an extra argument to those to tell them
to not allow reg+offset addressing when we know it's not allowed?

Peter

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Alan Modra]

On Sat, Jul 09, 2016 at 10:26:55AM -0500, Peter Bergner wrote:
> The problem occurs when we have an altivec store, which is what the original
> test case had:
> 
> bergner@genoa:~/gcc/BUGS/sawdey/altivec$ cat vec_store.i
> typedef __attribute__((altivec(vector__))) __attribute__((aligned(16))) unsigned char vec_t;
> void
> foo (vec_t *dst, vec_t src)
> {
>   dst[1] = src;
> }

The reason this fails is that no alternative in altivec_mov<mode>
exactly matches.  Ideally reload would choose the Z,v alternative
(cost 9) and you'd get an address reload for the mem to make it match
the "Z" constraint.  Instead reload chooses the Y,r alternative (cost
8) as best.

That's a bad choice.  "Y" matches the r3+16 mem so no reloads needed
for the mem, however the altivec reg needs to be reloaded to gprs.
Without direct moves from altivec to gpr the reload needs to go via
mem, so you get an altivec store to stack plus gpr load from stack.
The store to stack has the same form as the original store.  Oops, no
progress.

Even if direct moves were available from altivec regs to gprs, the Y,r
alternative would still be a bad choice.

I believe all the r alternatives in altivec_mov<mode> should be
disparaged with "?".

diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
index 7dad61e..65cbd6e 100644
--- a/gcc/config/rs6000/altivec.md
+++ b/gcc/config/rs6000/altivec.md
@@ -222,7 +222,7 @@
 
 ;; Vector move instructions.
 (define_insn "*altivec_mov<mode>"
-  [(set (match_operand:VM2 0 "nonimmediate_operand" "=Z,v,v,*Y,*r,*r,v,v,*r")
+  [(set (match_operand:VM2 0 "nonimmediate_operand" "=Z,v,v,*?Y,*?r,*?r,v,v,*?r")
 	(match_operand:VM2 1 "input_operand" "v,Z,v,r,Y,r,j,W,W"))]
   "VECTOR_MEM_ALTIVEC_P (<MODE>mode)
    && (register_operand (operands[0], <MODE>mode) 

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Segher Boessenkool]

On Mon, Jul 11, 2016 at 04:07:11PM +0930, Alan Modra wrote:
> I believe all the r alternatives in altivec_mov<mode> should be
> disparaged with "?".

That certainly makes sense.

Okay for trunk if tested (also test with LRA, if you can).  Thanks!


Segher

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Ulrich Weigand]

Alan Modra wrote:
> The reason this fails is that no alternative in altivec_mov<mode>
> exactly matches.  Ideally reload would choose the Z,v alternative
> (cost 9) and you'd get an address reload for the mem to make it match
> the "Z" constraint.  Instead reload chooses the Y,r alternative (cost
> 8) as best.
> 
> That's a bad choice.  "Y" matches the r3+16 mem so no reloads needed
> for the mem, however the altivec reg needs to be reloaded to gprs.
> Without direct moves from altivec to gpr the reload needs to go via
> mem, so you get an altivec store to stack plus gpr load from stack.
> The store to stack has the same form as the original store.  Oops, no
> progress.
> 
> Even if direct moves were available from altivec regs to gprs, the Y,r
> alternative would still be a bad choice.
> 
> I believe all the r alternatives in altivec_mov<mode> should be
> disparaged with "?".

I agree that this makes sense just from a code quality perspective.

However, there still seems to be another underlying problem: reload
should never generate invalid instructions.  (Playing with '?' to
fix *inefficient* instructions is fine, but we shouldn't rely on '?'
to fix *invalid* instructions.)

If reload -for whatever reason- wants to emit a vr->gpr move, then it
should generate a valid instruction sequence to do so, via secondary
reloads if necessary.  I think it would be a good idea to investigate
why that isn't happening in this test case.  [ There is a chance that
the underlying bug will reappear in a different context, even after
the '?' change is applied. ]

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Alan Modra]

On Mon, Jul 11, 2016 at 03:26:46PM +0200, Ulrich Weigand wrote:
> However, there still seems to be another underlying problem: reload
> should never generate invalid instructions.  (Playing with '?' to
> fix *inefficient* instructions is fine, but we shouldn't rely on '?'
> to fix *invalid* instructions.)
> 
> If reload -for whatever reason- wants to emit a vr->gpr move, then it
> should generate a valid instruction sequence to do so, via secondary
> reloads if necessary.  I think it would be a good idea to investigate
> why that isn't happening in this test case.  [ There is a chance that
> the underlying bug will reappear in a different context, even after
> the '?' change is applied. ]

The vr->gpr move uses secondary memory.  The memory allocated by
assign_stack_local, called from get_secondary_memory is

(mem/c:V16QI (plus:DI (reg/f:DI 113 sfp)
        (const_int 32 [0x20])) [0  S16 A128])

and that is incorrectly simplified by eliminate_regs to

(mem/c:V16QI (reg/f:DI 1 1))

for use by the strict_memory_address_space_p check, which of course
then passes.

eliminate_regs sees this entry in the elimination table:
(gdb) p *ep
$7 = {from = 113, to = 1, initial_offset = -32, can_eliminate = 1, can_eliminate_previous = 1, offset = -32, previous_offset = -32, ref_outside_mem = 0, from_rtx = 0x7ffff688a300, to_rtx = 0x7ffff688a2e8}

The offset in the elimination table was correct before
get_secondary_memory called assign_stack_local, but not after
frame_offset was changed when allocating a new stack slot.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Ulrich Weigand]

Alan Modra wrote:
> On Mon, Jul 11, 2016 at 03:26:46PM +0200, Ulrich Weigand wrote:
> > However, there still seems to be another underlying problem: reload
> > should never generate invalid instructions.  (Playing with '?' to
> > fix *inefficient* instructions is fine, but we shouldn't rely on '?'
> > to fix *invalid* instructions.)
> > 
> > If reload -for whatever reason- wants to emit a vr->gpr move, then it
> > should generate a valid instruction sequence to do so, via secondary
> > reloads if necessary.  I think it would be a good idea to investigate
> > why that isn't happening in this test case.  [ There is a chance that
> > the underlying bug will reappear in a different context, even after
> > the '?' change is applied. ]
> 
> The vr->gpr move uses secondary memory.  The memory allocated by
> assign_stack_local, called from get_secondary_memory is
> 
> (mem/c:V16QI (plus:DI (reg/f:DI 113 sfp)
>         (const_int 32 [0x20])) [0  S16 A128])
> 
> and that is incorrectly simplified by eliminate_regs to
> 
> (mem/c:V16QI (reg/f:DI 1 1))
> 
> for use by the strict_memory_address_space_p check, which of course
> then passes.
> 
> eliminate_regs sees this entry in the elimination table:
> (gdb) p *ep
> $7 = {from = 113, to = 1, initial_offset = -32, can_eliminate = 1, can_eliminate_previous = 1, offset = -32, previous_offset = -32, ref_outside_mem = 0, from_rtx = 0x7ffff688a300, to_rtx = 0x7ffff688a2e8}
> 
> The offset in the elimination table was correct before
> get_secondary_memory called assign_stack_local, but not after
> frame_offset was changed when allocating a new stack slot.

OK, I see.  Yes, the first time around that will be wrong.
However, if get_secondary_mem called assign_stack_local, the
stack size will be changing, which is why the main loop
around calculate_needs_all_insns() in reload() should set
the "something_changed" flag and restart the whole process.
(Which means the reloads for the first pass will be wrong,
but that doesn't matter since they will be ignored.)

The second time around, get_secondary_mem should reuse the
same stack slot it already allocated, and the elimination
offsets should already be set to accommodate that stack slot,
which means the second time around, the correct RTX should be
generated for the memory access.

Is this not happening somehow?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Alan Modra]

On Tue, Jul 12, 2016 at 02:02:43PM +0200, Ulrich Weigand wrote:
> The second time around, get_secondary_mem should reuse the
> same stack slot it already allocated, and the elimination
> offsets should already be set to accommodate that stack slot,
> which means the second time around, the correct RTX should be
> generated for the memory access.
> 
> Is this not happening somehow?

Duh, yes, of course.  Second time around the mem is
(mem/c:V16QI (plus:DI (reg/f:DI 1 1)
        (const_int -16 [0xfffffffffffffff0])) [0  S16 A128])
so we're checking the correct offset.

The problem now is that this passes rs6000_legitimate_address_p due to
mode_supports_vsx_dform_quad and quad_address_p being true.  That
doesn't seem correct for -mno-vsx.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Ulrich Weigand]

Alan Modra wrote:
> On Tue, Jul 12, 2016 at 02:02:43PM +0200, Ulrich Weigand wrote:
> > The second time around, get_secondary_mem should reuse the
> > same stack slot it already allocated, and the elimination
> > offsets should already be set to accommodate that stack slot,
> > which means the second time around, the correct RTX should be
> > generated for the memory access.
> > 
> > Is this not happening somehow?
> 
> Duh, yes, of course.  Second time around the mem is
> (mem/c:V16QI (plus:DI (reg/f:DI 1 1)
>         (const_int -16 [0xfffffffffffffff0])) [0  S16 A128])
> so we're checking the correct offset.
> 
> The problem now is that this passes rs6000_legitimate_address_p due to
> mode_supports_vsx_dform_quad and quad_address_p being true.  That
> doesn't seem correct for -mno-vsx.

Hmm, I see in rs6000_option_override_internal:

  /* ISA 3.0 D-form instructions require p9-vector and upper-regs.  */
  if ((TARGET_P9_DFORM_SCALAR || TARGET_P9_DFORM_VECTOR) && !TARGET_P9_VECTOR)
    {
      if (rs6000_isa_flags_explicit & OPTION_MASK_P9_VECTOR)
        error ("-mpower9-dform requires -mpower9-vector");
      rs6000_isa_flags &= ~(OPTION_MASK_P9_DFORM_SCALAR
                            | OPTION_MASK_P9_DFORM_VECTOR);
    }


and *later*:

  /* ISA 3.0 vector instructions include ISA 2.07.  */
  if (TARGET_P9_VECTOR && !TARGET_P8_VECTOR)
    {
      if (rs6000_isa_flags_explicit & OPTION_MASK_P8_VECTOR)
        error ("-mpower9-vector requires -mpower8-vector");
      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
    }

That seems the wrong way around ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Alan Modra]

On Tue, Jul 12, 2016 at 04:17:23PM +0200, Ulrich Weigand wrote:
> Alan Modra wrote:
> > The problem now is that this passes rs6000_legitimate_address_p due to
> > mode_supports_vsx_dform_quad and quad_address_p being true.  That
> > doesn't seem correct for -mno-vsx.
> 
> Hmm, I see in rs6000_option_override_internal:
> 
>   /* ISA 3.0 D-form instructions require p9-vector and upper-regs.  */
>   if ((TARGET_P9_DFORM_SCALAR || TARGET_P9_DFORM_VECTOR) && !TARGET_P9_VECTOR)
>     {
>       if (rs6000_isa_flags_explicit & OPTION_MASK_P9_VECTOR)
>         error ("-mpower9-dform requires -mpower9-vector");
>       rs6000_isa_flags &= ~(OPTION_MASK_P9_DFORM_SCALAR
>                             | OPTION_MASK_P9_DFORM_VECTOR);
>     }
> 
> 
> and *later*:
> 
>   /* ISA 3.0 vector instructions include ISA 2.07.  */
>   if (TARGET_P9_VECTOR && !TARGET_P8_VECTOR)
>     {
>       if (rs6000_isa_flags_explicit & OPTION_MASK_P8_VECTOR)
>         error ("-mpower9-vector requires -mpower8-vector");
>       rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
>     }
> 
> That seems the wrong way around ...

Yes, your analysis agrees with mine.  Putting these overrides in the
correct order will result in -mno-vsx forcing -mno-power9-dform (via
-mno-power8-vector and -mno-power9-vector).  With that in place, the
address predicates give the correct answer, and what's more, result in
reload costing altivec_mov alternatives the way we'd expect without
any changes to altivec_mov.

Incidentally, disparaging altivec_mov alternatives as I did in
https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00475.html cause
testsuite regressions on powerpc64le-linux.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Alan Modra]

This is what I've bootstrapped and regression tested on
powerpc64le-linux.  I'm using Peter's testcases from this thread
rather than the one in the original patch submission, because that one
relies on -O0 not reducing the function down to a nop.  OK to apply?

	PR target/71733
gcc/
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Deal
	with p9_vector override before power9-dform override.
gcc/testsuite/
	* gcc.target/powerpc/p9-novsx.c: New.

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 99a2e36..63655b1 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4256,6 +4256,14 @@ rs6000_option_override_internal (bool global_init_p)
       && !(rs6000_isa_flags_explicit & OPTION_MASK_TOC_FUSION))
     rs6000_isa_flags |= OPTION_MASK_TOC_FUSION;
 
+  /* ISA 3.0 vector instructions include ISA 2.07.  */
+  if (TARGET_P9_VECTOR && !TARGET_P8_VECTOR)
+    {
+      if (rs6000_isa_flags_explicit & OPTION_MASK_P8_VECTOR)
+	error ("-mpower9-vector requires -mpower8-vector");
+      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
+    }
+
   /* -mpower9-dform turns on both -mpower9-dform-scalar and
       -mpower9-dform-vector.  */
   if (TARGET_P9_DFORM_BOTH > 0)
@@ -4298,14 +4306,6 @@ rs6000_option_override_internal (bool global_init_p)
       rs6000_isa_flags &= ~OPTION_MASK_P9_DFORM_SCALAR;
     }
 
-  /* ISA 3.0 vector instructions include ISA 2.07.  */
-  if (TARGET_P9_VECTOR && !TARGET_P8_VECTOR)
-    {
-      if (rs6000_isa_flags_explicit & OPTION_MASK_P8_VECTOR)
-	error ("-mpower9-vector requires -mpower8-vector");
-      rs6000_isa_flags &= ~OPTION_MASK_P9_VECTOR;
-    }
-
   /* There have been bugs with -mvsx-timode that don't show up with -mlra,
      but do show up with -mno-lra.  Given -mlra will become the default once
      PR 69847 is fixed, turn off the options with problems by default if
diff --git a/gcc/testsuite/gcc.target/powerpc/p9-novsx.c b/gcc/testsuite/gcc.target/powerpc/p9-novsx.c
new file mode 100644
index 0000000..7e41030
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/p9-novsx.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power9" } } */
+/* { dg-options "-mcpu=power9 -mno-vsx -O1" } */
+/* { dg-require-effective-target powerpc_altivec_ok } */
+/* { dg-final { scan-assembler-times "lvx %?v?2,%?r?3" 1 } } */
+/* { dg-final { scan-assembler-times "stvx %?v?2,%?r?3" 1 } } */
+
+/* PR target/71733.  */
+typedef __attribute__ ((altivec(vector__), aligned(16))) unsigned char vec_t;
+
+vec_t
+f1 (vec_t *dst)
+{
+  return dst[1];
+}
+
+void
+f2 (vec_t *dst, vec_t src)
+{
+  dst[1] = src;
+}


-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Segher Boessenkool]

On Wed, Jul 13, 2016 at 02:57:01PM +0930, Alan Modra wrote:
> This is what I've bootstrapped and regression tested on
> powerpc64le-linux.  I'm using Peter's testcases from this thread
> rather than the one in the original patch submission, because that one
> relies on -O0 not reducing the function down to a nop.  OK to apply?

This is okay.  Does it need a backport?  Okay for 6 as well, then.

We all agreed the question marks would be a good idea, but you say it
causes some testcases to fail.  Could you investigate please?

Thanks,


Segher


> 	PR target/71733
> gcc/
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Deal
> 	with p9_vector override before power9-dform override.
> gcc/testsuite/
> 	* gcc.target/powerpc/p9-novsx.c: New.

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Alan Modra]

On Wed, Jul 13, 2016 at 06:46:22AM -0500, Segher Boessenkool wrote:
> On Wed, Jul 13, 2016 at 02:57:01PM +0930, Alan Modra wrote:
> > This is what I've bootstrapped and regression tested on
> > powerpc64le-linux.  I'm using Peter's testcases from this thread
> > rather than the one in the original patch submission, because that one
> > relies on -O0 not reducing the function down to a nop.  OK to apply?
> 
> This is okay.  Does it need a backport?  Okay for 6 as well, then.

Yes, gcc-6 needs the patch too.

> We all agreed the question marks would be a good idea, but you say it
> causes some testcases to fail.  Could you investigate please?

After much head-scratching (danger of splinters) I noticed that I'd
written '*?r' in the patch rather than '?*r'.  That explained failures
like gcc.target/powerpc/ppc-vector-memset.c using gprs rather than vrs
for the memset expansion.  According to md.text, '*' says the
following char should be ignored when choosing register preferences.
(Which is a bug, since the advent of multi-char constraints, and
potentially affects us with our use of constraint strings like "?*wb".)
Anyway, what was ignored for reg allocation was the '?', not 'r'.

Also, '*Y' is a bit pointless since 'Y' isn't a register constraint.
The '*' belongs on the corresponding operand 1 'r'.

I also saw ICEs in rs6000_split_multireg_move on a number of
gcc.dg/vmx testcases, but the ICEs disappeared with the constraints
fixed.  I can't give you the exact rtl involved now since my debug
session had a connection timeout, but it was this assert:
		  gcc_assert (GET_CODE (XEXP (dst, 0)) == PLUS
			      && REG_P (basereg)
			      && REG_P (offsetreg)
			      && REGNO (basereg) != REGNO (offsetreg));
and we had an altivec MEM with an AND address for the destination of a
SET with gprs for the source.  Probably quite easily fixed by
stripping off the AND.

The following has now been bootstrapped and regression tested on
powerpc64le-linux.  OK for mainline?

	* gcc/config/rs6000/altivec.md (altivec_mov<mode>): Disparage
	gpr alternatives.  Correct '*' placement on Y,r alternative.
	Add '*' on operand 1 of r,r alternative.

diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
index aa01ac9..9193f07 100644
--- a/gcc/config/rs6000/altivec.md
+++ b/gcc/config/rs6000/altivec.md
@@ -222,8 +222,8 @@
 
 ;; Vector move instructions.
 (define_insn "*altivec_mov<mode>"
-  [(set (match_operand:VM2 0 "nonimmediate_operand" "=Z,v,v,*Y,*r,*r,v,v,*r")
-	(match_operand:VM2 1 "input_operand" "v,Z,v,r,Y,r,j,W,W"))]
+  [(set (match_operand:VM2 0 "nonimmediate_operand" "=Z,v,v,?Y,?*r,?*r,v,v,?*r")
+	(match_operand:VM2 1 "input_operand" "v,Z,v,*r,Y,*r,j,W,W"))]
   "VECTOR_MEM_ALTIVEC_P (<MODE>mode)
    && (register_operand (operands[0], <MODE>mode) 
        || register_operand (operands[1], <MODE>mode))"

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Alan Modra]

On Thu, Jul 14, 2016 at 12:14:10PM +0930, Alan Modra wrote:
> (Which is a bug, since the advent of multi-char constraints, and
> potentially affects us with our use of constraint strings like "?*wb".)

On looking into this, I see it is just a documentation bug.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Segher Boessenkool]

On Thu, Jul 14, 2016 at 12:14:10PM +0930, Alan Modra wrote:
> The following has now been bootstrapped and regression tested on
> powerpc64le-linux.  OK for mainline?
> 
> 	* gcc/config/rs6000/altivec.md (altivec_mov<mode>): Disparage
> 	gpr alternatives.  Correct '*' placement on Y,r alternative.
> 	Add '*' on operand 1 of r,r alternative.

Okay for mainline, thank you!


Segher

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Peter Bergner]

On 7/12/16 8:48 AM, Alan Modra wrote:
> On Tue, Jul 12, 2016 at 02:02:43PM +0200, Ulrich Weigand wrote:
>> The second time around, get_secondary_mem should reuse the
>> same stack slot it already allocated, and the elimination
>> offsets should already be set to accommodate that stack slot,
>> which means the second time around, the correct RTX should be
>> generated for the memory access.
>>
>> Is this not happening somehow?
>
> Duh, yes, of course.  Second time around the mem is
> (mem/c:V16QI (plus:DI (reg/f:DI 1 1)
>         (const_int -16 [0xfffffffffffffff0])) [0  S16 A128])
> so we're checking the correct offset.
>
> The problem now is that this passes rs6000_legitimate_address_p due to
> mode_supports_vsx_dform_quad and quad_address_p being true.  That
> doesn't seem correct for -mno-vsx.

Catching up with email, not that I'm back from vacation.

This still doesn't answer David's question about what will happen if
we generate this pattern (or one of the older VSX reg+reg patterns)
when we are NOT using -mno-vsx.  In those cases, quad_address_p and
mode_supports_vsx_dform_quad will return true and it seems like
we'll go ahead and generate these reg+offset addresses when they're
not legal for these patterns.

As I said in my previous note, I wasn't able to actually generate the
altivec pattern (I haven't tried the vsx reg+reg patterns), but if we
could, I assume we'll still have the same issue, will we not?

Peter

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Alan Modra]

On Wed, Jul 20, 2016 at 08:51:21PM -0500, Peter Bergner wrote:
> This still doesn't answer David's question about what will happen if
> we generate this pattern (or one of the older VSX reg+reg patterns)
> when we are NOT using -mno-vsx.  In those cases, quad_address_p and
> mode_supports_vsx_dform_quad will return true and it seems like
> we'll go ahead and generate these reg+offset addresses when they're
> not legal for these patterns.

The question doesn't make a great deal of sense.  altivec_mov* and
vsx_mov* have mutually exclusive insn predicates, and at the RTL level
they all look the same.  So even if you did generate the "wrong"
insn RTL, the right one would match.  For example, if you generated
RTL using altivec_mov with -mvsx, vsx_mov would match it, *not*
altivec_mov.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, rs6000] Fix PR target/71733, ICE with -mcpu=power9 -mno-vsx

[Peter Bergner]

On 7/21/16 3:26 AM, Alan Modra wrote:
> On Wed, Jul 20, 2016 at 08:51:21PM -0500, Peter Bergner wrote:
>> This still doesn't answer David's question about what will happen if
>> we generate this pattern (or one of the older VSX reg+reg patterns)
>> when we are NOT using -mno-vsx.  In those cases, quad_address_p and
>> mode_supports_vsx_dform_quad will return true and it seems like
>> we'll go ahead and generate these reg+offset addresses when they're
>> not legal for these patterns.
>
> The question doesn't make a great deal of sense.  altivec_mov* and
> vsx_mov* have mutually exclusive insn predicates, and at the RTL level
> they all look the same.  So even if you did generate the "wrong"
> insn RTL, the right one would match.  For example, if you generated
> RTL using altivec_mov with -mvsx, vsx_mov would match it, *not*
> altivec_mov.

Ah, so you're saying we'll never generate those patterns (modulo thru
builtins which already force reg+reg addressing), so this isn't a
problem?  Well that answers our question then.  Thanks!

Peter