[rs6000] Fix PR83969: ICE in final_scan_insn, at final.c:2997

Message ID a90d0b0e-c605-747e-1a47-572a59fb2977@vnet.ibm.com
State New
Headers show
Series
  • [rs6000] Fix PR83969: ICE in final_scan_insn, at final.c:2997
Related show

Commit Message

Peter Bergner March 8, 2018, 12:50 a.m.
PR83969 shows another bug in mem_operand_gpr() (which implements the "Y"
constraint) accepting reg+reg addresses.  This was fixed by adding a call
to rs6000_offsettable_memref_p() to verify the address is a valid offsettable
address.  Fixing that exposed a problem in the *movdi_internal64 pattern
which was using the "Y" constraint for the integer load/store alternatives,
which allow either reg+offset or reg+reg addresses.  This worked before
only because of the buggy mem_operand_gpr.  The fix for that was to use
the "YZ" constraint which allows both reg+offset and reg+reg addresses.

This passed bootstrap and regtesting on powerpc64-linux, running the
testsuite in both 32-bit and 64-bit modes with no regressions.
Ok for trunk?

Peter

gcc/
    PR target/83969
    * config/rs6000/rs6000.c (rs6000_offsettable_memref_p): New prototype.
    Add strict argument and use it.
    (rs6000_split_multireg_move): Update for new strict argument.
    (mem_operand_gpr): Disallow all non-offsettable addresses.
    * config/rs6000/rs6000.md (*movdi_internal64): Use YZ constraint.

gcc/testsuite/
    PR target/83969
    * gcc.target/powerpc/pr83969.c: New test.

+}

Comments

Peter Bergner March 8, 2018, 1:03 p.m. | #1
On 3/7/18 6:50 PM, Peter Bergner wrote:
> This passed bootstrap and regtesting on powerpc64-linux, running the

> testsuite in both 32-bit and 64-bit modes with no regressions.

> Ok for trunk?


FYI, this also passed bootstrap and regtesting on powerpc64le-linux
without regressions also.

Peter
Segher Boessenkool March 9, 2018, 7:31 p.m. | #2
On Wed, Mar 07, 2018 at 06:50:41PM -0600, Peter Bergner wrote:
> PR83969 shows another bug in mem_operand_gpr() (which implements the "Y"

> constraint) accepting reg+reg addresses.  This was fixed by adding a call

> to rs6000_offsettable_memref_p() to verify the address is a valid offsettable

> address.  Fixing that exposed a problem in the *movdi_internal64 pattern

> which was using the "Y" constraint for the integer load/store alternatives,

> which allow either reg+offset or reg+reg addresses.  This worked before

> only because of the buggy mem_operand_gpr.  The fix for that was to use

> the "YZ" constraint which allows both reg+offset and reg+reg addresses.

> 

> This passed bootstrap and regtesting on powerpc64-linux, running the

> testsuite in both 32-bit and 64-bit modes with no regressions.

> Ok for trunk?


Sorry this took a while to review.  It is okay for trunk.  Does this
need backports?

Thanks!


Segher


> gcc/

>     PR target/83969

>     * config/rs6000/rs6000.c (rs6000_offsettable_memref_p): New prototype.

>     Add strict argument and use it.

>     (rs6000_split_multireg_move): Update for new strict argument.

>     (mem_operand_gpr): Disallow all non-offsettable addresses.

>     * config/rs6000/rs6000.md (*movdi_internal64): Use YZ constraint.

> 

> gcc/testsuite/

>     PR target/83969

>     * gcc.target/powerpc/pr83969.c: New test.
Peter Bergner March 9, 2018, 10:25 p.m. | #3
On 3/9/18 1:31 PM, Segher Boessenkool wrote:
> On Wed, Mar 07, 2018 at 06:50:41PM -0600, Peter Bergner wrote:

>> This passed bootstrap and regtesting on powerpc64-linux, running the

>> testsuite in both 32-bit and 64-bit modes with no regressions.

>> Ok for trunk?

> 

> Sorry this took a while to review.  It is okay for trunk.  Does this

> need backports?


Technically, it is broken there too, but until trunk, we never really
generated the altivec mems that trigger this bug, so I think I would
lean towards just having this on trunk and if someone, somehow hits
it, then we can back port it then.

Peter
Peter Bergner April 19, 2018, 6:23 p.m. | #4
On 3/9/18 4:25 PM, Peter Bergner wrote:
> On 3/9/18 1:31 PM, Segher Boessenkool wrote:

>> On Wed, Mar 07, 2018 at 06:50:41PM -0600, Peter Bergner wrote:

>>> This passed bootstrap and regtesting on powerpc64-linux, running the

>>> testsuite in both 32-bit and 64-bit modes with no regressions.

>>> Ok for trunk?

>>

>> Sorry this took a while to review.  It is okay for trunk.  Does this

>> need backports?

> 

> Technically, it is broken there too, but until trunk, we never really

> generated the altivec mems that trigger this bug, so I think I would

> lean towards just having this on trunk and if someone, somehow hits

> it, then we can back port it then.


So as we talked offline, the go test case in PR85436 is fixed by this
patch, so I have back ported it and am bootstrapping and regtesting it.
Is it ok for GCC 7 if the testing comes back clean?

Peter
Segher Boessenkool April 19, 2018, 10:40 p.m. | #5
On Thu, Apr 19, 2018 at 01:23:51PM -0500, Peter Bergner wrote:
> On 3/9/18 4:25 PM, Peter Bergner wrote:

> > On 3/9/18 1:31 PM, Segher Boessenkool wrote:

> >> On Wed, Mar 07, 2018 at 06:50:41PM -0600, Peter Bergner wrote:

> >>> This passed bootstrap and regtesting on powerpc64-linux, running the

> >>> testsuite in both 32-bit and 64-bit modes with no regressions.

> >>> Ok for trunk?

> >>

> >> Sorry this took a while to review.  It is okay for trunk.  Does this

> >> need backports?

> > 

> > Technically, it is broken there too, but until trunk, we never really

> > generated the altivec mems that trigger this bug, so I think I would

> > lean towards just having this on trunk and if someone, somehow hits

> > it, then we can back port it then.

> 

> So as we talked offline, the go test case in PR85436 is fixed by this

> patch, so I have back ported it and am bootstrapping and regtesting it.

> Is it ok for GCC 7 if the testing comes back clean?


Certainly, thanks.  Is it needed for GCC 6 as well?  Okay for that too,
if so.


Segher
Peter Bergner April 20, 2018, 2:34 p.m. | #6
On 4/19/18 5:40 PM, Segher Boessenkool wrote:
> On Thu, Apr 19, 2018 at 01:23:51PM -0500, Peter Bergner wrote:

>> On 3/9/18 4:25 PM, Peter Bergner wrote:

>>> Technically, it is broken there too, but until trunk, we never really

>>> generated the altivec mems that trigger this bug, so I think I would

>>> lean towards just having this on trunk and if someone, somehow hits

>>> it, then we can back port it then.

>>

>> So as we talked offline, the go test case in PR85436 is fixed by this

>> patch, so I have back ported it and am bootstrapping and regtesting it.

>> Is it ok for GCC 7 if the testing comes back clean?

> 

> Certainly, thanks.  Is it needed for GCC 6 as well?  Okay for that too,

> if so.

Ok, the following is what I ended up committing.  Thanks.

Neither test case fails on GCC 6, but I haven't tested on BE which is
where the PR83969 test case was failing (-m32 BE).  I'll do a build on
BE and verify whether the tests compile there or not.  If they PASS,
I'd probably just leave it alone until someone comes up with a test
case that FAILs on GCC 6 before back porting it.

Peter


gcc/
	Backport from mainline
	2018-03-09  Peter Bergner  <bergner@vnet.ibm.com>

	PR target/83969
	* config/rs6000/rs6000.c (rs6000_offsettable_memref_p): New prototype.
	Add strict argument and use it.
	(rs6000_split_multireg_move): Update for new strict argument.
	(mem_operand_gpr): Disallow all non-offsettable addresses.
	* config/rs6000/rs6000.md (*movdi_internal64): Use YZ constraint.

gcc/testsuite/
	PR target/85436
	* go.dg/pr85436.go: New test.

	Backport from mainline
	2018-03-09  Peter Bergner  <bergner@vnet.ibm.com>

	PR target/83969
	* gcc.target/powerpc/pr83969.c: New test.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 259519)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -1372,6 +1372,7 @@ static rtx rs6000_debug_legitimize_reloa
 						   int, int, int *);
 static bool rs6000_mode_dependent_address (const_rtx);
 static bool rs6000_debug_mode_dependent_address (const_rtx);
+static bool rs6000_offsettable_memref_p (rtx, machine_mode, bool);
 static enum reg_class rs6000_secondary_reload_class (enum reg_class,
 						     machine_mode, rtx);
 static enum reg_class rs6000_debug_secondary_reload_class (enum reg_class,
@@ -8564,10 +8565,8 @@ 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)
+  /* Don't allow non-offsettable addresses.  See PRs 83969 and 84279.  */
+  if (!rs6000_offsettable_memref_p (op, mode, false))
     return false;
 
   op = address_offset (addr);
@@ -10340,7 +10339,7 @@ rs6000_find_base_term (rtx op)
    in 32-bit mode, that the recog predicate rejects.  */
 
 static bool
-rs6000_offsettable_memref_p (rtx op, machine_mode reg_mode)
+rs6000_offsettable_memref_p (rtx op, machine_mode reg_mode, bool strict)
 {
   bool worst_case;
 
@@ -10348,7 +10347,7 @@ rs6000_offsettable_memref_p (rtx op, mac
     return false;
 
   /* First mimic offsettable_memref_p.  */
-  if (offsettable_address_p (true, GET_MODE (op), XEXP (op, 0)))
+  if (offsettable_address_p (strict, GET_MODE (op), XEXP (op, 0)))
     return true;
 
   /* offsettable_address_p invokes rs6000_mode_dependent_address, but
@@ -10362,7 +10361,7 @@ rs6000_offsettable_memref_p (rtx op, mac
   worst_case = ((TARGET_POWERPC64 && GET_MODE_CLASS (reg_mode) == MODE_INT)
 		|| GET_MODE_SIZE (reg_mode) == 4);
   return rs6000_legitimate_offset_address_p (GET_MODE (op), XEXP (op, 0),
-					     true, worst_case);
+					     strict, worst_case);
 }
 
 /* Determine the reassociation width to be used in reassociate_bb.
@@ -26559,7 +26558,7 @@ rs6000_split_multireg_move (rtx dst, rtx
 	      emit_insn (gen_add3_insn (breg, breg, delta_rtx));
 	      src = replace_equiv_address (src, breg);
 	    }
-	  else if (! rs6000_offsettable_memref_p (src, reg_mode))
+	  else if (! rs6000_offsettable_memref_p (src, reg_mode, true))
 	    {
 	      if (GET_CODE (XEXP (src, 0)) == PRE_MODIFY)
 		{
@@ -26626,7 +26625,7 @@ rs6000_split_multireg_move (rtx dst, rtx
 		emit_insn (gen_add3_insn (breg, breg, delta_rtx));
 	      dst = replace_equiv_address (dst, breg);
 	    }
-	  else if (!rs6000_offsettable_memref_p (dst, reg_mode)
+	  else if (!rs6000_offsettable_memref_p (dst, reg_mode, true)
 		   && GET_CODE (XEXP (dst, 0)) != LO_SUM)
 	    {
 	      if (GET_CODE (XEXP (dst, 0)) == PRE_MODIFY)
@@ -26665,7 +26664,7 @@ rs6000_split_multireg_move (rtx dst, rtx
 		}
 	    }
 	  else if (GET_CODE (XEXP (dst, 0)) != LO_SUM)
-	    gcc_assert (rs6000_offsettable_memref_p (dst, reg_mode));
+	    gcc_assert (rs6000_offsettable_memref_p (dst, reg_mode, true));
 	}
 
       for (i = 0; i < nregs; i++)
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 259519)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -8640,14 +8640,14 @@ (define_split
 ;;              FPR->GPR   GPR->FPR   VSX->GPR   GPR->VSX
 (define_insn "*movdi_internal64"
   [(set (match_operand:DI 0 "nonimmediate_operand"
-               "=Y,        r,         r,         r,         r,          r,
+               "=YZ,       r,         r,         r,         r,          r,
                 ^m,        ^d,        ^d,        ^wY,       $Z,         $wb,
                 $wv,       ^wi,       *wo,       *wo,       *wv,        *wi,
                 *wi,       *wv,       *wv,       r,         *h,         *h,
                 ?*r,       ?*wg,      ?*r,       ?*wj")
 
 	(match_operand:DI 1 "input_operand"
-                "r,        Y,         r,         I,         L,          nF,
+                "r,        YZ,        r,         I,         L,          nF,
                  d,        m,         d,         wb,        wv,         wY,
                  Z,        wi,        Oj,        wM,        OjwM,       Oj,
                  wM,       wS,        wB,        *h,        r,          0,
Index: gcc/testsuite/gcc.target/powerpc/pr83969.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr83969.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/pr83969.c	(working copy)
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=G5" } } */
+/* { dg-options "-O1 -mcpu=G5 -fno-split-wide-types -ftree-loop-vectorize" } */
+
+long long int
+n7 (int po, long long int r4)
+{
+  while (po < 1)
+    {
+      r4 |= 1;
+      ++po;
+    }
+  return r4;
+}
Index: gcc/testsuite/go.dg/pr85436.go
===================================================================
--- gcc/testsuite/go.dg/pr85436.go	(nonexistent)
+++ gcc/testsuite/go.dg/pr85436.go	(working copy)
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -mcpu=power9" { target { powerpc*-*-* } } } */
+
+package main
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+)
+type testFuncs struct { }
+func (t *testFuncs) load(filename, pkg string, doImport, seen *bool) {
+	var testFileSet = token.NewFileSet()
+	f, err := parser.ParseFile(testFileSet, filename, nil, parser.ParseComments)
+	if err != nil { }
+	for _, d := range f.Decls {
+		n, ok := d.(*ast.FuncDecl)
+		if !ok { }
+		ptr := n.Type.Params.List[0].Type.(*ast.StarExpr)
+		if sel := ptr.X.(*ast.SelectorExpr); sel.Sel.Name == "M" { }
+	}
+}
Peter Bergner April 20, 2018, 3:44 p.m. | #7
On 4/20/18 9:34 AM, Peter Bergner wrote:
> Neither test case fails on GCC 6, but I haven't tested on BE which is

> where the PR83969 test case was failing (-m32 BE).  I'll do a build on

> BE and verify whether the tests compile there or not.  If they PASS,

> I'd probably just leave it alone until someone comes up with a test

> case that FAILs on GCC 6 before back porting it.


OK, GCC 6 on BE doesn't ICE on either test case, so I'm going to just
leave things as they are there.  We can always back port the fixes later
if someone comes up with yet another test case that fails there.

Peter

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c    (revision 258268)
+++ gcc/config/rs6000/rs6000.c    (working copy)
@@ -1378,6 +1378,7 @@  static rtx rs6000_debug_legitimize_reloa
                            int, int, int *);
 static bool rs6000_mode_dependent_address (const_rtx);
 static bool rs6000_debug_mode_dependent_address (const_rtx);
+static bool rs6000_offsettable_memref_p (rtx, machine_mode, bool);
 static enum reg_class rs6000_secondary_reload_class (enum reg_class,
                              machine_mode, rtx);
 static enum reg_class rs6000_debug_secondary_reload_class (enum reg_class,
@@ -8207,10 +8208,8 @@  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)
+  /* Don't allow non-offsettable addresses.  See PRs 83969 and 84279.  */
+  if (!rs6000_offsettable_memref_p (op, mode, false))
     return false;
 
   op = address_offset (addr);
@@ -9956,7 +9955,7 @@  rs6000_find_base_term (rtx op)
    in 32-bit mode, that the recog predicate rejects.  */
 
 static bool
-rs6000_offsettable_memref_p (rtx op, machine_mode reg_mode)
+rs6000_offsettable_memref_p (rtx op, machine_mode reg_mode, bool strict)
 {
   bool worst_case;
 
@@ -9964,7 +9963,7 @@  rs6000_offsettable_memref_p (rtx op, mac
     return false;
 
   /* First mimic offsettable_memref_p.  */
-  if (offsettable_address_p (true, GET_MODE (op), XEXP (op, 0)))
+  if (offsettable_address_p (strict, GET_MODE (op), XEXP (op, 0)))
     return true;
 
   /* offsettable_address_p invokes rs6000_mode_dependent_address, but
@@ -9978,7 +9977,7 @@  rs6000_offsettable_memref_p (rtx op, mac
   worst_case = ((TARGET_POWERPC64 && GET_MODE_CLASS (reg_mode) == MODE_INT)
         || GET_MODE_SIZE (reg_mode) == 4);
   return rs6000_legitimate_offset_address_p (GET_MODE (op), XEXP (op, 0),
-                         true, worst_case);
+                         strict, worst_case);
 }
 
 /* Determine the reassociation width to be used in reassociate_bb.
@@ -24063,7 +24062,7 @@  rs6000_split_multireg_move (rtx dst, rtx
           emit_insn (gen_add3_insn (breg, breg, delta_rtx));
           src = replace_equiv_address (src, breg);
         }
-      else if (! rs6000_offsettable_memref_p (src, reg_mode))
+      else if (! rs6000_offsettable_memref_p (src, reg_mode, true))
         {
           if (GET_CODE (XEXP (src, 0)) == PRE_MODIFY)
         {
@@ -24130,7 +24129,7 @@  rs6000_split_multireg_move (rtx dst, rtx
         emit_insn (gen_add3_insn (breg, breg, delta_rtx));
           dst = replace_equiv_address (dst, breg);
         }
-      else if (!rs6000_offsettable_memref_p (dst, reg_mode)
+      else if (!rs6000_offsettable_memref_p (dst, reg_mode, true)
            && GET_CODE (XEXP (dst, 0)) != LO_SUM)
         {
           if (GET_CODE (XEXP (dst, 0)) == PRE_MODIFY)
@@ -24169,7 +24168,7 @@  rs6000_split_multireg_move (rtx dst, rtx
         }
         }
       else if (GET_CODE (XEXP (dst, 0)) != LO_SUM)
-        gcc_assert (rs6000_offsettable_memref_p (dst, reg_mode));
+        gcc_assert (rs6000_offsettable_memref_p (dst, reg_mode, true));
     }
 
       for (i = 0; i < nregs; i++)
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md    (revision 258268)
+++ gcc/config/rs6000/rs6000.md    (working copy)
@@ -8549,14 +8549,14 @@  (define_split
 ;;              FPR->GPR   GPR->FPR   VSX->GPR   GPR->VSX
 (define_insn "*movdi_internal64"
   [(set (match_operand:DI 0 "nonimmediate_operand"
-               "=Y,        r,         r,         r,         r,          r,
+               "=YZ,       r,         r,         r,         r,          r,
                 ^m,        ^d,        ^d,        ^wY,       $Z,         $wb,
                 $wv,       ^wi,       *wo,       *wo,       *wv,        *wi,
                 *wi,       *wv,       *wv,       r,         *h,         *h,
                 ?*r,       ?*wg,      ?*r,       ?*wj")
 
     (match_operand:DI 1 "input_operand"
-                "r,        Y,         r,         I,         L,          nF,
+                "r,        YZ,        r,         I,         L,          nF,
                  d,        m,         d,         wb,        wv,         wY,
                  Z,        wi,        Oj,        wM,        OjwM,       Oj,
                  wM,       wS,        wB,        *h,        r,          0,
Index: gcc/testsuite/gcc.target/powerpc/pr83969.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr83969.c    (revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr83969.c    (working copy)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } {
"-mcpu=G5" } } */
+/* { dg-options "-O1 -mcpu=G5 -fno-split-wide-types -ftree-loop-vectorize" } */
+
+long long int
+n7 (int po, long long int r4)
+{
+  while (po < 1)
+    {
+      r4 |= 1;
+      ++po;
+    }
+  return r4;