diff mbox series

combine: Fix split_i2i3 ICE [PR94291]

Message ID 20200331070418.GB2212@tucnak
State New
Headers show
Series combine: Fix split_i2i3 ICE [PR94291] | expand

Commit Message

Li, Pan2 via Gcc-patches March 31, 2020, 7:04 a.m. UTC 
Hi!

The following testcase ICEs on armv7hl-linux-gnueabi.
try_combine is called on:
(gdb) p debug_rtx (i3)
(insn 20 12 22 2 (set (mem/c:SI (plus:SI (reg/f:SI 102 sfp)
                (const_int -4 [0xfffffffffffffffc])) [1 x+0 S4 A32])
        (reg:SI 125)) "pr94291.c":7:8 241 {*arm_movsi_insn}
     (expr_list:REG_DEAD (reg:SI 125)
        (nil)))
(gdb) p debug_rtx (i2)
(insn 12 7 20 2 (parallel [
            (set (reg:CC 100 cc)
                (compare:CC (reg:SI 121 [ <retval> ])
                    (const_int 0 [0])))
            (set (reg:SI 125)
                (reg:SI 121 [ <retval> ]))
        ]) "pr94291.c":7:8 248 {*movsi_compare0}
     (expr_list:REG_UNUSED (reg:CC 100 cc)
        (nil)))
and tries to recognize cc = r121 cmp 0; [sfp-4] = r121 parallel,
but that isn't recognized, so it splits it into two: split_i2i3
[sfp-4] = r121 followed by cc = r121 cmp 0 which is recognized, but
ICEs because the code below insist that the SET_DEST of newi2pat
(or first set in PARALLEL thereof) must be a REG or SUBREG of REG,
but it is a MEM in this case.  I don't see any condition that would
guarantee that, perhaps for the swap_i2i3 case it was somehow guaranteed.

As the code just wants to update LOG_LINKS and LOG_LINKS are only for
registers, not for MEM or anything else, the patch just doesn't update those
if it isn't a REG or SUBREG of REG.

Bootstrapped/regtested on armv7hl-linux-gnueabi and powerpc64le-linux, ok
for trunk?

2020-03-31  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/94291
	* combine.c (try_combine): For split_i2i3, don't assume SET_DEST
	must be a REG or SUBREG of REG; if it is not one of these, don't
	update LOG_LINKs.

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


	Jakub

Comments

Li, Pan2 via Gcc-patches April 7, 2020, 10:53 a.m. UTC | #1 
Hi!

I'd like to ping this P1 PR fix:

On Tue, Mar 31, 2020 at 09:04:18AM +0200, Jakub Jelinek via Gcc-patches wrote:
> 2020-03-31  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/94291
> 	* combine.c (try_combine): For split_i2i3, don't assume SET_DEST
> 	must be a REG or SUBREG of REG; if it is not one of these, don't
> 	update LOG_LINKs.
> 
> 	* gcc.dg/pr94291.c: New test.

	Jakub
Segher Boessenkool April 7, 2020, 7:18 p.m. UTC | #2 
Hi!

(I have no idea why this PR is a P1).

On Tue, Mar 31, 2020 at 09:04:18AM +0200, Jakub Jelinek wrote:
> The following testcase ICEs on armv7hl-linux-gnueabi.

Also on default arm-linux-gnueabi.  It needs -Og.

> try_combine is called on:
> (gdb) p debug_rtx (i3)
> (insn 20 12 22 2 (set (mem/c:SI (plus:SI (reg/f:SI 102 sfp)
>                 (const_int -4 [0xfffffffffffffffc])) [1 x+0 S4 A32])
>         (reg:SI 125)) "pr94291.c":7:8 241 {*arm_movsi_insn}
>      (expr_list:REG_DEAD (reg:SI 125)
>         (nil)))
> (gdb) p debug_rtx (i2)
> (insn 12 7 20 2 (parallel [
>             (set (reg:CC 100 cc)
>                 (compare:CC (reg:SI 121 [ <retval> ])
>                     (const_int 0 [0])))
>             (set (reg:SI 125)
>                 (reg:SI 121 [ <retval> ]))
>         ]) "pr94291.c":7:8 248 {*movsi_compare0}
>      (expr_list:REG_UNUSED (reg:CC 100 cc)
>         (nil)))
> and tries to recognize cc = r121 cmp 0; [sfp-4] = r121 parallel,
> but that isn't recognized,

Trying 12 -> 20:
   12: {cc:CC=cmp(r118:SI,0);r122:SI=r118:SI;}
      REG_UNUSED cc:CC
   20: [sfp:SI-0x4]=r122:SI
      REG_DEAD r122:SI
Failed to match this instruction:
(parallel [
        (set (reg:CC 100 cc)
            (compare:CC (reg:SI 118 [ <retval> ])
                (const_int 0 [0])))
        (set (mem/c:SI (plus:SI (reg/f:SI 102 sfp)
                    (const_int -4 [0xfffffffffffffffc])) [1 x+0 S4 A32])
            (reg:SI 118 [ <retval> ]))
    ])
(twice)

> so it splits it into two: split_i2i3

(That variable does not *control* the splitting: it registers that we
did split a parallel into newi2pat and newpat).

Successfully matched this instruction:
(set (mem/c:SI (plus:SI (reg/f:SI 102 sfp)
            (const_int -4 [0xfffffffffffffffc])) [1 x+0 S4 A32])
    (reg:SI 118 [ <retval> ]))
Successfully matched this instruction:
(set (reg:CC 100 cc)
    (compare:CC (reg:SI 118 [ <retval> ])
        (const_int 0 [0])))
allowing combination of insns 12 and 20
original costs 4 + 4 = 8
replacement costs 4 + 4 = 8

> [sfp-4] = r121 followed by cc = r121 cmp 0 which is recognized, but
> ICEs because the code below insist that the SET_DEST of newi2pat
> (or first set in PARALLEL thereof) must be a REG or SUBREG of REG,
> but it is a MEM in this case.  I don't see any condition that would
> guarantee that, perhaps for the swap_i2i3 case it was somehow guaranteed.

I don't see how it was guaranteed for that case, either.

> As the code just wants to update LOG_LINKS and LOG_LINKS are only for
> registers, not for MEM or anything else, the patch just doesn't update those
> if it isn't a REG or SUBREG of REG.

That is correct.

> 2020-03-31  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/94291

Please mention PR rtl-optimization/84169 as well.

> 	* combine.c (try_combine): For split_i2i3, don't assume SET_DEST
> 	must be a REG or SUBREG of REG; if it is not one of these, don't
> 	update LOG_LINKs.
> 
> 	* gcc.dg/pr94291.c: New test.

Okay for trunk, and for backports as well (after soaking it for a bit).
Thanks!


Segher
diff mbox series

Patch

--- gcc/combine.c.jj	2020-02-25 13:56:19.514076091 +0100
+++ gcc/combine.c	2020-03-30 17:23:17.579654895 +0200
@@ -4351,25 +4351,29 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
       if (GET_CODE (x) == PARALLEL)
 	x = XVECEXP (newi2pat, 0, 0);
 
-      /* It can only be a SET of a REG or of a SUBREG of a REG.  */
-      unsigned int regno = reg_or_subregno (SET_DEST (x));
-
-      bool done = false;
-      for (rtx_insn *insn = NEXT_INSN (i3);
-	   !done
-	   && insn
-	   && NONDEBUG_INSN_P (insn)
-	   && BLOCK_FOR_INSN (insn) == this_basic_block;
-	   insn = NEXT_INSN (insn))
+      if (REG_P (SET_DEST (x))
+	  || (GET_CODE (SET_DEST (x)) == SUBREG
+	      && REG_P (SUBREG_REG (SET_DEST (x)))))
 	{
-	  struct insn_link *link;
-	  FOR_EACH_LOG_LINK (link, insn)
-	    if (link->insn == i3 && link->regno == regno)
-	      {
-		link->insn = i2;
-		done = true;
-		break;
-	      }
+	  unsigned int regno = reg_or_subregno (SET_DEST (x));
+
+	  bool done = false;
+	  for (rtx_insn *insn = NEXT_INSN (i3);
+	       !done
+	       && insn
+	       && NONDEBUG_INSN_P (insn)
+	       && BLOCK_FOR_INSN (insn) == this_basic_block;
+	       insn = NEXT_INSN (insn))
+	    {
+	      struct insn_link *link;
+	      FOR_EACH_LOG_LINK (link, insn)
+		if (link->insn == i3 && link->regno == regno)
+		  {
+		    link->insn = i2;
+		    done = true;
+		    break;
+		  }
+	    }
 	}
     }
 
--- gcc/testsuite/gcc.dg/pr94291.c.jj	2020-03-30 17:22:47.876092906 +0200
+++ gcc/testsuite/gcc.dg/pr94291.c	2020-03-30 17:22:22.230471072 +0200
@@ -0,0 +1,14 @@ 
+/* PR rtl-optimization/94291 */
+/* { dg-do compile } */
+/* { dg-options "-Og" } */
+
+unsigned a;
+
+unsigned
+foo (void)
+{
+  unsigned x
+    = (__builtin_sub_overflow ((long long) a, 0, &x)
+       ? 1 : (__INTPTR_TYPE__) __builtin_memmove (&x, foo, 1));
+  return a;
+}