Re: [PATCH] cselib, reload: Fix cselib ICE on m68k/microblaze [PR94526]

2020-04-08 Thread Jeff Law via Gcc-patches
On Wed, 2020-04-08 at 20:23 +0200, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase ICEs on m68k (and another one Jeff mailed me
> privately on microblaze).
> The problem is that reload creates two DEBUG_INSNs with the same
> value of (plus:P (reg:P sp) (const_int 0)), we compute correctly the
> same hash value for them, but then don't find them in the cselib hash table,
> as rtx_equal_for_cselib_1 thinks it is different from (reg:P sp),
> and trigger an assertion failure that requires that from two different debug
> insns one doesn't add locations to VALUEs.
> 
> The patch has two fixes for this, each fixes the ICE on both targets
> separately, but I think we want both.
> 
> The cselib.c change ensures that rtx_equal_for_cselib_1 considers
> (value:P sp_derived_value) and (plus:P (reg:P sp) (const_int 0)) equivalent.
> 
> The reload1.c change makes sure we don't create those bogus plus 0
> expressions.  I understand the reasons for creating them, but they don't
> really apply to DEBUG_INSNs; we don't have validity matching there, all we
> care is that the expressions aren't arbitrarily deep, but it is just fine
> to fold x + 0 into just x in there.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, tested on the
> testcases with crosses to m86k-linux and microblaze-linux, ok for trunk?
> 
> 2020-04-08  Jakub Jelinek  
> 
>   PR middle-end/94526
>   * cselib.c (autoinc_split): Handle e->val_rtx being SP_DERIVED_VALUE_P
>   with zero offset.
>   * reload1.c (eliminate_regs_1): Avoid creating
>   (plus (reg) (const_int 0)) in DEBUG_INSNs.
> 
>   * gcc.dg/pr94526.c: New test.
OK
jeff
> 



[PATCH] cselib, reload: Fix cselib ICE on m68k/microblaze [PR94526]

2020-04-08 Thread Jakub Jelinek via Gcc-patches
Hi!

The following testcase ICEs on m68k (and another one Jeff mailed me
privately on microblaze).
The problem is that reload creates two DEBUG_INSNs with the same
value of (plus:P (reg:P sp) (const_int 0)), we compute correctly the
same hash value for them, but then don't find them in the cselib hash table,
as rtx_equal_for_cselib_1 thinks it is different from (reg:P sp),
and trigger an assertion failure that requires that from two different debug
insns one doesn't add locations to VALUEs.

The patch has two fixes for this, each fixes the ICE on both targets
separately, but I think we want both.

The cselib.c change ensures that rtx_equal_for_cselib_1 considers
(value:P sp_derived_value) and (plus:P (reg:P sp) (const_int 0)) equivalent.

The reload1.c change makes sure we don't create those bogus plus 0
expressions.  I understand the reasons for creating them, but they don't
really apply to DEBUG_INSNs; we don't have validity matching there, all we
care is that the expressions aren't arbitrarily deep, but it is just fine
to fold x + 0 into just x in there.

Bootstrapped/regtested on x86_64-linux and i686-linux, tested on the
testcases with crosses to m86k-linux and microblaze-linux, ok for trunk?

2020-04-08  Jakub Jelinek  

PR middle-end/94526
* cselib.c (autoinc_split): Handle e->val_rtx being SP_DERIVED_VALUE_P
with zero offset.
* reload1.c (eliminate_regs_1): Avoid creating
(plus (reg) (const_int 0)) in DEBUG_INSNs.

* gcc.dg/pr94526.c: New test.

--- gcc/cselib.c.jj 2020-04-07 13:18:00.935059416 +0200
+++ gcc/cselib.c2020-04-08 13:03:15.105090429 +0200
@@ -884,21 +884,29 @@ autoinc_split (rtx x, rtx *off, machine_
   else
e = cselib_lookup (x, GET_MODE (x), 0, memmode);
   if (e)
-   for (struct elt_loc_list *l = e->locs; l; l = l->next)
- if (GET_CODE (l->loc) == PLUS
- && GET_CODE (XEXP (l->loc, 0)) == VALUE
- && SP_DERIVED_VALUE_P (XEXP (l->loc, 0))
- && CONST_INT_P (XEXP (l->loc, 1)))
+   {
+ if (SP_DERIVED_VALUE_P (e->val_rtx)
+ && (*off == NULL_RTX || *off == const0_rtx))
{
- if (*off == NULL_RTX)
-   *off = XEXP (l->loc, 1);
- else
-   *off = plus_constant (Pmode, *off,
- INTVAL (XEXP (l->loc, 1)));
- if (*off == const0_rtx)
-   *off = NULL_RTX;
- return XEXP (l->loc, 0);
+ *off = NULL_RTX;
+ return e->val_rtx;
}
+ for (struct elt_loc_list *l = e->locs; l; l = l->next)
+   if (GET_CODE (l->loc) == PLUS
+   && GET_CODE (XEXP (l->loc, 0)) == VALUE
+   && SP_DERIVED_VALUE_P (XEXP (l->loc, 0))
+   && CONST_INT_P (XEXP (l->loc, 1)))
+ {
+   if (*off == NULL_RTX)
+ *off = XEXP (l->loc, 1);
+   else
+ *off = plus_constant (Pmode, *off,
+   INTVAL (XEXP (l->loc, 1)));
+   if (*off == const0_rtx)
+ *off = NULL_RTX;
+   return XEXP (l->loc, 0);
+ }
+   }
 }
   return x;
 }
--- gcc/reload1.c.jj2020-01-12 11:54:36.923405714 +0100
+++ gcc/reload1.c   2020-04-08 12:41:38.508513800 +0200
@@ -2607,8 +2607,9 @@ eliminate_regs_1 (rtx x, machine_mode me
   structure of the insn in a way that reload can't handle.
   We special-case the commonest situation in
   eliminate_regs_in_insn, so just replace a PLUS with a
-  PLUS here, unless inside a MEM.  */
-   if (mem_mode != 0
+  PLUS here, unless inside a MEM.  In DEBUG_INSNs, it is
+  always ok to replace a PLUS with just a REG.  */
+   if ((mem_mode != 0 || (insn && DEBUG_INSN_P (insn)))
&& CONST_INT_P (XEXP (x, 1))
&& known_eq (INTVAL (XEXP (x, 1)), -ep->previous_offset))
  return ep->to_rtx;
--- gcc/testsuite/gcc.dg/pr94526.c.jj   2020-04-08 12:42:01.423170364 +0200
+++ gcc/testsuite/gcc.dg/pr94526.c  2020-04-08 12:41:09.664946098 +0200
@@ -0,0 +1,21 @@
+/* PR middle-end/94526 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -g" } */
+
+struct S { int val[8 * sizeof (int)]; };
+
+void
+foo (struct S *x)
+{
+  struct S *a = x;
+}
+
+void baz (struct S);
+
+void
+bar (void)
+{
+  struct S b;
+  foo ();
+  baz (b);
+}

Jakub