[rs6000] Fix PR84279, powerpc64le ICE on cvc4

Message ID 31e557e6-4b62-eeca-a1a3-173397c205e7@bergner.org
State New
Headers show
Series
  • [rs6000] Fix PR84279, powerpc64le ICE on cvc4
Related show

Commit Message

Peter Bergner Feb. 13, 2018, 3:34 a.m.
PR84279 is a similar problem to PR83399, in that we generate an altivec
load/store through an explicit call to the altivec_{l,st}vx_v4si_2op
pattern and then due to spilling, we end up calling recog() and we match
an earlier pattern, in this case vsx_movv4si_64bit.  That is ok, since
this pattern can generate the lvx/stvx insns the altivec patterm can.
However, due to a constraint bug, we end up using the wrong alternative.

The problematic code after spilling looks like:

(insn 92 131 126 2 (parallel [
            (set (reg:V4SI 140)
                (unspec:V4SI [
                        (reg:SI 143 [ g ])
                        (reg:SI 150 [ ar.v ])
                        (subreg:SI (reg:DI 146) 0)
                        (subreg:SI (reg:DI 149) 0)
                    ] UNSPEC_VSX_VEC_INIT))
            (clobber (scratch:DI))
            (clobber (scratch:DI))
        ]) "bug.i":25 1237 {vsx_init_v4si})

(insn 126 92 95 2 (set (mem/c:V4SI (and:DI (plus:DI (reg/f:DI 111 sfp)
                    (reg:DI 156))
                (const_int -16 [0xfffffffffffffff0])) [3 MEM[(struct A *)&am]+0 S16 A128])
        (reg:V4SI 140)) "bug.i":25 1792 {altivec_stvx_v4si_2op})

The vsx_init_v4si pattern forces pseudo 140 to be assigned a GPR, which
should force a reload in insn 126, because the altivec store requires
an altivec register for its src operand.  However, after recog(), we
end up using the vsx_movv4si_64bit pattern which looks like:

(define_insn "*vsx_mov<mode>_64bit"
  [(set (match_operand:VSX_M 0 "nonimmediate_operand"
               "=ZwO,      <VSa>,     <VSa>,     r,         we,        ?wQ,
                ?&r,       ??r,       ??Y,       ??r,       wo,        v,
                ?<VSa>,    *r,        v,         ??r,       wZ,        v")

        (match_operand:VSX_M 1 "input_operand" 
               "<VSa>,     ZwO,       <VSa>,     we,        r,         r,
                wQ,        Y,         r,         r,         wE,        jwM,
                ?jwM,      jwM,       W,         W,         v,         wZ"))]

Now we _should_ match using the second to last alternative, but we end up
matching the 8th alternative ("??Y" and "r").  The 8th alternative is used for
storing a GPR, which we have, but the mem we're trying to store to does not
have a valid address for a GPR store.  The "bug" is that the "Y" constraint
code, which is implemented by  mem_operand_gpr() allows our altivec address
when it should not.  The following patch which fixes the ICE adds code to
mem_operand_gpr() which disallows such addresses.

This patch passed bootstrap and retesting on powerpc64le-linux with
no regressions.  Ok for mainline?

Peter

gcc/
	PR target/84279
	* config/rs6000/rs6000.c (mem_operand_gpr): Disallow altivec addresses.

gcc/testsuite/
	PR target/84279
	* g++.dg/pr84279.C: New test.

Comments

Segher Boessenkool Feb. 13, 2018, 10:34 p.m. | #1
Hi!

On Mon, Feb 12, 2018 at 09:34:30PM -0600, Peter Bergner wrote:
> PR84279 is a similar problem to PR83399, in that we generate an altivec
> load/store through an explicit call to the altivec_{l,st}vx_v4si_2op
> pattern and then due to spilling, we end up calling recog() and we match
> an earlier pattern, in this case vsx_movv4si_64bit.  That is ok, since
> this pattern can generate the lvx/stvx insns the altivec patterm can.
> However, due to a constraint bug, we end up using the wrong alternative.

> Now we _should_ match using the second to last alternative, but we end up
> matching the 8th alternative ("??Y" and "r").  The 8th alternative is used for
> storing a GPR, which we have, but the mem we're trying to store to does not
> have a valid address for a GPR store.  The "bug" is that the "Y" constraint
> code, which is implemented by  mem_operand_gpr() allows our altivec address
> when it should not.  The following patch which fixes the ICE adds code to
> mem_operand_gpr() which disallows such addresses.
> 
> This patch passed bootstrap and retesting on powerpc64le-linux with
> no regressions.  Ok for mainline?

Okay, thanks!  Does this need backports?

> --- gcc/testsuite/g++.dg/pr84279.C	(nonexistent)
> +++ gcc/testsuite/g++.dg/pr84279.C	(working copy)
> @@ -0,0 +1,35 @@
> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */

I don't think this needs lp64?


Segher
Peter Bergner Feb. 13, 2018, 11:07 p.m. | #2
On 2/13/18 4:34 PM, Segher Boessenkool wrote:
>> This patch passed bootstrap and retesting on powerpc64le-linux with
>> no regressions.  Ok for mainline?
> 
> Okay, thanks!  Does this need backports?

Committed with your suggested change below.  Thanks!

It'd be easy to backport and should be fairly harmless.  That said, I was
never able to create a simpler test case, using __builtin_altivec_lvx()
that would end up being re-recog'd as vsx_movv4si_64, so the only way I
know we can hit this is with Kelvin's optimization that replaces aligned
vsx loads/stores with altivec loads/stores and that optimization is only
on trunk.

It's up to you whether you want the backport because you don't trust
me being able to create a failing test case. :-)


>> --- gcc/testsuite/g++.dg/pr84279.C	(nonexistent)
>> +++ gcc/testsuite/g++.dg/pr84279.C	(working copy)
>> @@ -0,0 +1,35 @@
>> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
> 
> I don't think this needs lp64?

Yeah, I think you're right.  I'll remove it.  Thanks.

Peter
Segher Boessenkool Feb. 13, 2018, 11:51 p.m. | #3
On Tue, Feb 13, 2018 at 05:07:26PM -0600, Peter Bergner wrote:
> On 2/13/18 4:34 PM, Segher Boessenkool wrote:
> >> This patch passed bootstrap and retesting on powerpc64le-linux with
> >> no regressions.  Ok for mainline?
> > 
> > Okay, thanks!  Does this need backports?
> 
> Committed with your suggested change below.  Thanks!
> 
> It'd be easy to backport and should be fairly harmless.  That said, I was
> never able to create a simpler test case, using __builtin_altivec_lvx()
> that would end up being re-recog'd as vsx_movv4si_64, so the only way I
> know we can hit this is with Kelvin's optimization that replaces aligned
> vsx loads/stores with altivec loads/stores and that optimization is only
> on trunk.
> 
> It's up to you whether you want the backport because you don't trust
> me being able to create a failing test case. :-)

We can backport without having a failing testcase.  Let's do that for 7
at least?


Segher
Peter Bergner Feb. 14, 2018, 1:12 a.m. | #4
On 2/13/18 5:51 PM, Segher Boessenkool wrote:
> On Tue, Feb 13, 2018 at 05:07:26PM -0600, Peter Bergner wrote:
>> It's up to you whether you want the backport because you don't trust
>> me being able to create a failing test case. :-)
> 
> We can backport without having a failing testcase.  Let's do that for 7
> at least?

Ok, I'll test the back port to GCC 7 and commit if no regressions.

Peter
Peter Bergner Feb. 14, 2018, 3:18 p.m. | #5
On 2/13/18 5:51 PM, Segher Boessenkool wrote:
> We can backport without having a failing testcase.  Let's do that for 7
> at least?

Ok, the backport tested clean, so I committed it.  Thanks.

Peter

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 257606)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -8220,6 +8220,12 @@  mem_operand_gpr (rtx op, machine_mode mo
   int extra;
   rtx addr = XEXP (op, 0);
 
+  /* Don't allow altivec type addresses like (mem (and (plus ...))).
+     See PR target/84279.  */
+
+  if (GET_CODE (addr) == AND)
+    return false;
+
   op = address_offset (addr);
   if (op == NULL_RTX)
     return true;
Index: gcc/testsuite/g++.dg/pr84279.C
===================================================================
--- gcc/testsuite/g++.dg/pr84279.C	(nonexistent)
+++ gcc/testsuite/g++.dg/pr84279.C	(working copy)
@@ -0,0 +1,35 @@ 
+/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
+/* { dg-options "-O3 -mcpu=power8 -g -fPIC -fvisibility=hidden -fstack-protector-strong" } */
+
+template <typename, typename T> struct E { T e; };
+struct J {
+  unsigned k, l;
+  J (unsigned x, unsigned y) : k(x), l(y) {}
+};
+typedef struct A {
+  J n, p;
+  A ();
+  A (J x, J y) : n(x), p(y) {}
+} *S;
+S t;
+struct B {
+  struct C {
+    S q, r;
+    int u, v;
+    bool m1 (S, A &);
+    J m2 () const;
+    J m3 () const;
+    A m4 () const;
+  };
+  typedef E<unsigned, S> D;
+  void m5 (D *);
+  void m6 (unsigned, A);
+};
+bool B::C::m1 (S, A &x) { bool o; x = m4 (); return o; }
+J B::C::m2 () const { unsigned g (u == 0); unsigned h (v); return J (g, h); }
+J B::C::m3 () const { unsigned g (q != t); unsigned h (r != t); return J (g, h); }
+A B::C::m4 () const { return A (m2 (), m3 ()); }
+void B::m5 (D *c) { unsigned x; C ar; A am; if (ar.m1 (c->e, am)) m6 (x, am); }