This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[RFA][rtl-optimization/52714] Do not allow combine to create invalid RTL



As mentioned in the PR, we call try_combine with:


i1 = (insn 14 13 16 2 (set (reg/v/f:SI 34 [ stack ])
        (reg/f:SI 15 %sp)) pr52714.c:9 38 {*movsi_m68k2}
     (nil))
i2 = (insn 16 14 17 2 (set (cc0)
        (compare (reg/v/f:SI 34 [ stack ])
            (const_int 0 [0]))) pr52714.c:10 4 {*tstsi_internal_68020_cf}
     (nil))
i3 = (jump_insn 17 16 18 2 (set (pc)
        (if_then_else (eq (cc0)
                (const_int 0 [0]))
            (label_ref 40)
            (pc))) pr52714.c:10 390 {beq}
     (int_list:REG_BR_PROB 1014 (nil))
 -> 40)

This is then combined into:

newpat = (parallel [
        (set (pc)
            (pc))
        (set (reg/v/f:SI 34 [ stack ])
            (reg/f:SI 15 %sp))
    ])

This isn't a recognized insn, and it needs to be split. Since it's just a parallel of independent sets, combine tries to split it into a pair of assignments which look like:

(insn 16 14 17 2 (set (pc)
        (pc)) pr52714.c:10 2147483647 {NOOP_MOVE}
     (nil))
(jump_insn 17 16 18 2 (set (reg/v/f:SI 34 [ stack ])
        (reg/f:SI 15 %sp)) pr52714.c:10 38 {*movsi_m68k2}
     (int_list:REG_BR_PROB 1014 (nil))
 -> 40)


Note how the second is a JUMP_INSN, but it doesn't modify the PC.  Opps.


ISTM the code in combine which tries to rip apart a PARALLEL like that needs to ensure that I2 and I3 are both INSNs.

The astute reader would notice that this wasn't optimized at the tree level. That's an artifact of using -O1 instead of -O2. At -O2 VRP would come in and zap that test well before it ever expanded into RTL.

Verified all 3 tests in the PR are fixed (two full, one reduced). Reduced test to be added to the test suite.

Bootstrap and regression test in progress on x86_64-unknown-linux-gnu. OK if that passes without incident?



diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index e489b62..7983139 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2014-02-06  Jeff Law  <law@redhat.com>
+
+	PR rtl-optimization/52714
+	* combine.c (try_combine): When splitting an unrecognized PARALLEL
+	into two independent simple sets, make sure both I3 and I2 are
+	INSNs (as opposed to JUMP_INSNs or CALL_INSNs).
+
 2014-02-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
 
 	* config/arm/aarch-cost-tables.h (cortexa57_extra_costs): New table.
diff --git a/gcc/combine.c b/gcc/combine.c
index fd4294b..12db3a3 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -3692,7 +3692,11 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx i0, int *new_direct_jump_p,
 	   && ! reg_referenced_p (SET_DEST (XVECEXP (newpat, 0, 0)),
 				  XVECEXP (newpat, 0, 1))
 	   && ! (contains_muldiv (SET_SRC (XVECEXP (newpat, 0, 0)))
-		 && contains_muldiv (SET_SRC (XVECEXP (newpat, 0, 1)))))
+		 && contains_muldiv (SET_SRC (XVECEXP (newpat, 0, 1))))
+	   /* Make sure both I2 and I3 are simple INSNs, otherwise we might
+	      create a JUMP_INSN with a PATTERN that is a simple set.  */
+	   && GET_CODE (i2) == INSN
+	   && GET_CODE (i3) == INSN)
     {
       rtx set0 = XVECEXP (newpat, 0, 0);
       rtx set1 = XVECEXP (newpat, 0, 1);
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 2cac8d2..5d6e066 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-02-06  Jeff Law  <law@redhat.com>
+
+	PR rtl-optimization/52714
+	* gcc.c-torture/compile/pr52714.c: New test.
+
 2014-02-06  Jakub Jelinek  <jakub@redhat.com>
 
 	PR target/59575
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52714.c b/gcc/testsuite/gcc.c-torture/compile/pr52714.c
new file mode 100644
index 0000000..03d4990
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr52714.c
@@ -0,0 +1,25 @@
+
+int __re_compile_fastmap(unsigned char *p)
+{
+    unsigned char **stack;
+    unsigned size;
+    unsigned avail;
+
+    stack = __builtin_alloca(5 * sizeof(unsigned char*));
+    if (stack == 0)
+	return -2;
+    size = 5;
+    avail = 0;
+
+    for (;;) {
+	switch (*p++) {
+	case 0:
+	    if (avail == size)
+		return -2;
+	    stack[avail++] = p;
+	}
+    }
+
+    return 0;
+}
+

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]