| Message ID | 560D2EC9.8010009@arm.com |
|---|---|
| State | New |
| Headers | show |
> This patch fixes the two wrong-code PRs. > The problem is related to the way the noce_emit_cmove helper function > emits conditional moves. > For some targets it re-emits the comparison from the condition block and > then the conditional move > after we have emitted the two basic blocks. Later passes always catch > the redundant comparison and eliminate > it anyway. However, this means that if any of the basic blocks clobber a > register > that is used in that comparison, the comparison will go wrong. > > This happens in the testcase where one of the intermediate insns in the > basic block re-used a pseudo reg > that was used in the comparison to store an intermediate result. When > the comparison was re-emitted by > noce_emit_cmove later, it used the clobbered pseudo reg. > > > PR rtl-optimization/67786 > PR rtl-optimization/67787 > * ifcvt.c (bb_valid_for_noce_process_p): Reject basic block if > it modifies a reg used in the condition calculation. To duoble-check its effect on code generation I ran this against my collection of .i files and it appears to have an effect only on the testcases, as expected. This is ok. Bernd
commit ee8b9f163dad61e43f9f53f1e6c4e224a3712095
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Thu Oct 1 09:37:27 2015 +0100
[RTL ifcvt] PR 67786, 67787: Check that intermediate instructions in the basic block don't clobber a reg used in condition
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index f280c64..8846e69 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3110,7 +3110,8 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond,
gcc_assert (sset);
if (contains_mem_rtx_p (SET_SRC (sset))
- || !REG_P (SET_DEST (sset)))
+ || !REG_P (SET_DEST (sset))
+ || reg_overlap_mentioned_p (SET_DEST (sset), cond))
goto free_bitmap_and_fail;
potential_cost += insn_rtx_cost (sset, speed_p);
diff --git a/gcc/testsuite/gcc.dg/pr67786.c b/gcc/testsuite/gcc.dg/pr67786.c
new file mode 100644
index 0000000..76525e5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr67786.c
@@ -0,0 +1,20 @@
+/* { dg-do run } */
+/* { dg-options "-O3" } */
+
+int a, b = 10;
+char c;
+
+int
+main ()
+{
+ char d;
+ int e = 5;
+ for (a = 0; a; a--)
+ e = 0;
+ c = (b & 15) ^ e;
+ d = c > e ? c : c << e;
+ __builtin_printf ("%d\n", d);
+ return 0;
+}
+
+/* { dg-output "15" } */
diff --git a/gcc/testsuite/gcc.dg/pr67787.c b/gcc/testsuite/gcc.dg/pr67787.c
new file mode 100644
index 0000000..238d7e3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr67787.c
@@ -0,0 +1,43 @@
+/* { dg-do run } */
+/* { dg-options "-O3" } */
+
+int a, c, f, g;
+char b;
+
+static int
+fn1 ()
+{
+ char h;
+ int k = -1, i, j;
+ for (; b < 16; b++)
+ ;
+ __builtin_printf (" ");
+ if (b < 5)
+ k++;
+ if (k)
+ {
+ int l = 2;
+ a = h = b < 0 || b > (127 >> l) ? b : b << 1;
+ return 0;
+ }
+ for (i = 0; i < 1; i++)
+ for (j = 0; j < 7; j++)
+ f = 0;
+ for (c = 0; c; c++)
+ ;
+ if (g)
+ for (;;)
+ ;
+ return 0;
+}
+
+int
+main ()
+{
+ fn1 ();
+
+ if (a != 32)
+ __builtin_abort ();
+
+ return 0;
+}
Hi all, This patch fixes the two wrong-code PRs. The problem is related to the way the noce_emit_cmove helper function emits conditional moves. For some targets it re-emits the comparison from the condition block and then the conditional move after we have emitted the two basic blocks. Later passes always catch the redundant comparison and eliminate it anyway. However, this means that if any of the basic blocks clobber a register that is used in that comparison, the comparison will go wrong. This happens in the testcase where one of the intermediate insns in the basic block re-used a pseudo reg that was used in the comparison to store an intermediate result. When the comparison was re-emitted by noce_emit_cmove later, it used the clobbered pseudo reg. There's no reason why the basic block should have used that pseudo-reg and not just used a fresh one, but that's what the previous passes produced (this takes place in ce2 after combine) and RTL is not in SSA form. Anyway, the simple way to deal with this is in bb_valid_for_noce_process_p to reject a SET destination that appears in the cond expression. This patch fixes the testcases and bootstrap and testing passes on arm, x86_64 and aarch64. Ok for trunk? Thanks, Kyrill 2015-10-01 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR rtl-optimization/67786 PR rtl-optimization/67787 * ifcvt.c (bb_valid_for_noce_process_p): Reject basic block if it modifies a reg used in the condition calculation. 2015-10-01 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * gcc.dg/pr67786.c: New test. * gcc.dg/pr67787.c: Likewise.