[PATCH][RTL ifcvt] PR 67786, 67787: Check that intermediate instructions in the basic block don't clobber a reg used in condition

[PATCH][RTL ifcvt] PR 67786, 67787: Check that intermediate instructions in the basic block don't clobber a reg used in condition

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifcvt-cond-clobber.patch --]
[-- Type: text/x-patch; name=ifcvt-cond-clobber.patch, Size: 2060 bytes --]

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;
+}

Re: [PATCH][RTL ifcvt] PR 67786, 67787: Check that intermediate instructions in the basic block don't clobber a reg used in condition

[Bernd Schmidt]

> 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