diff mbox

[RTL-ifcvt] PR rtl-optimization/68624: Clean up logic that checks for clobbering conflicts across basic blocks

Message ID 56600C6F.1010701@arm.com
State Accepted
Commit 1986d6791c18e5d9d527eb5acec1f326085a0ca7
Headers show

Commit Message

Kyrylo Tkachov Dec. 3, 2015, 9:33 a.m. UTC
Hi all,

In this fix I want to simplify the control flow of the code that chooses the order in which to emit
the then and else basic blocks (and their associated emit_a and emit_b instructions).
Currently we check the then block and only if there is a modification there we check the else block
and make a decision there. IMO it's much simpler if we check both blocks and write the logic that
chooses the order as a simple IF-ELSEIF-ELSE block that only emits the blocks and doesn't try to do
any other checks.  The bug in the logic that was preventing the clobber check from being performed
in this PR was in the code:
   if (emit_a || modified_in_a)
     {
       modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
       if (tmp_b && else_bb)
         {
           FOR_BB_INSNS (else_bb, tmp_insn)

where the second if condition should have been:
       if (tmp_a && else_bb)

Just changing the tmp_b to tmp_a in that condition would have fixed the wrong-code part of this PR
as we would have ended up rejecting if-conversion. However, there is a valid if-conversion opportunity
here, we just have to emit emit_a followed by else_bb, which the current control flow made awkward, which
is why I'm suggesting this small rewrite.

Bootstrapped and tested on x86_64, aarch64, arm.

Ok for trunk?
Thanks,
Kyrill

2015-12-03  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68624
     * ifcvt.c (noce_try_cmove_arith): Check clobbers of temp regs in both
     blocks if they exist and simplify the logic choosing the order to emit
     them in.

2015-12-03  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68624
     * gcc.c-torture/execute/pr68624.c: New test.
diff mbox

Patch

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 86b6ef7246ceddd223e93922737496af3d93f148..ef23c4cda66e6a659eee9b30089a6cc056cea30f 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2202,10 +2202,6 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
 	}
     }
 
-    /* If insn to set up A clobbers any registers B depends on, try to
-       swap insn that sets up A with the one that sets up B.  If even
-       that doesn't help, punt.  */
-
   modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
   if (tmp_b && then_bb)
     {
@@ -2220,31 +2216,33 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
 	  }
 
     }
-  if (emit_a || modified_in_a)
+
+  modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
+  if (tmp_a && else_bb)
     {
-      modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
-      if (tmp_b && else_bb)
+      FOR_BB_INSNS (else_bb, tmp_insn)
+      /* Don't check inside insn_b.  We will have changed it to emit_b
+	 with a destination that doesn't conflict.  */
+      if (!(insn_b && tmp_insn == insn_b)
+	  && modified_in_p (orig_a, tmp_insn))
 	{
-	  FOR_BB_INSNS (else_bb, tmp_insn)
-	  /* Don't check inside insn_b.  We will have changed it to emit_b
-	     with a destination that doesn't conflict.  */
-	  if (!(insn_b && tmp_insn == insn_b)
-	      && modified_in_p (orig_a, tmp_insn))
-	    {
-	      modified_in_b = true;
-	      break;
-	    }
+	  modified_in_b = true;
+	  break;
 	}
-      if (modified_in_b)
-	goto end_seq_and_fail;
+    }
 
+  /* If insn to set up A clobbers any registers B depends on, try to
+     swap insn that sets up A with the one that sets up B.  If even
+     that doesn't help, punt.  */
+  if (modified_in_a && !modified_in_b)
+    {
       if (!noce_emit_bb (emit_b, else_bb, b_simple))
 	goto end_seq_and_fail;
 
       if (!noce_emit_bb (emit_a, then_bb, a_simple))
 	goto end_seq_and_fail;
     }
-  else
+  else if (!modified_in_a)
     {
       if (!noce_emit_bb (emit_a, then_bb, a_simple))
 	goto end_seq_and_fail;
@@ -2252,6 +2250,8 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
       if (!noce_emit_bb (emit_b, else_bb, b_simple))
 	goto end_seq_and_fail;
     }
+  else
+    goto end_seq_and_fail;
 
   target = noce_emit_cmove (if_info, x, code, XEXP (if_info->cond, 0),
 			    XEXP (if_info->cond, 1), a, b);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68624.c b/gcc/testsuite/gcc.c-torture/execute/pr68624.c
new file mode 100644
index 0000000000000000000000000000000000000000..abb716b1550038cb3d0e96e8917b7ed0ba8bfa83
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68624.c
@@ -0,0 +1,30 @@ 
+int b, c, d, e = 1, f, g, h, j;
+
+static int
+fn1 ()
+{
+  int a = c;
+  if (h)
+    return 9;
+  g = (c || b) % e;
+  if ((g || f) && b)
+    return 9;
+  e = d;
+  for (c = 0; c > -4; c--)
+    ;
+  if (d)
+    c--;
+  j = c;
+  return d;
+}
+
+int
+main ()
+{
+  fn1 ();
+
+  if (c != -4)
+    __builtin_abort ();
+
+  return 0;
+}