diff mbox

[RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

Message ID 5657372D.4080907@arm.com
State Superseded
Headers show

Commit Message

Kyrylo Tkachov Nov. 26, 2015, 4:45 p.m. UTC 
On 26/11/15 14:35, Kyrill Tkachov wrote:
>

> On 26/11/15 14:23, Bernd Schmidt wrote:

>> On 11/26/2015 02:52 PM, Kyrill Tkachov wrote:

>>>

>>> On 26/11/15 13:40, Bernd Schmidt wrote:

>>>> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:

>>>>>       modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a,

>>>>> emit_b);

>>>>

>>>> Can this ever be true? We arrange for emit_b to set a new pseudo,

>>>> don't we? Are we allowing cases where we copy a pattern that sets more

>>>> than one register, and is that safe?

>>>

>>> You're right, this statement always sets modifieb_in_b to false. We

>>> reject anything bug single_set insns

>>> by this point in the code. I'll replace that with modified_in_b = false;

>>

>> Note that there's a mirrored test for modified_in_a, and both are already initialized to false.

>

> Yeah, that can be changed to just false too. I'll do that in the next revision.

>


Here is the updated patch.
I've reindented the if-else blocks and removed the always-false initialisation of modified_a and modified_b.

Re-tested on x86_64, aarch64.

Ok for trunk?

Thanks,
Kyrill

2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68506
     * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
     first if emit_a exists or then_bb modifies 'b'.  Reindent if-else
     blocks.

2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68506
     * gcc.c-torture/execute/pr68506.c: New test.


>> Also - careful with single_set, it can return true even for multiple sets in case there's a REG_DEAD note on one of them. You might want to strengthen your tests to also include !multiple_sets. Then, maybe instead of deleting these 

>> tests, turn them into gcc_checking_asserts.

>>

>

> I see. I think the best place to do that would be in insn_valid_noce_process_p and just get it to return

> false if multiple_sets (insn) is true.

>

> Would it be ok if I did that as a separate follow-up patch?

> We don't have a testcase where this actually causes trouble and I'd like to keep the fix for

> this PR as self-contained as possible.

>

> Thanks,

> Kyrill

>

>

>

>>

>> Bernd

>>

>

Comments

Kyrylo Tkachov Nov. 26, 2015, 4:54 p.m. UTC | #1 
On 26/11/15 16:49, Bernd Schmidt wrote:
> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:

>>          that doesn't help, punt.  */

>>

>> -  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);

>>     if (tmp_b && then_bb)

>>       {

> These bits I thought would be part of a followup patch (which would also guard against single_set problems), and as I mentioned I'd rather have a checking assert.

Yes, you're right. I have the checking_assert statement in the followup that I've been testing.
I'll move the deletion of these two statements there as well to minimise the changes to this patch.

I'll move these bits to that patch, re-build cc1 and commit.

Thanks for your guidance,
Kyrill

> So take these deletions out and leave them for the followup, and the patch is ok everywhere. No need for a full retest given that practically the same patch has been tested already, just make sure you can build cc1.

>

>

> Bernd

>
diff mbox

Patch

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index af7a3b96f38bea429f86916fc176913cac2e6ebc..9092b377e45082ec07a30d60b070575f4dc68641 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2206,7 +2206,6 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
        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)
     {
       FOR_BB_INSNS (then_bb, tmp_insn)
@@ -2220,40 +2219,37 @@  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_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))
-		{
-		  modified_in_b = true;
-		  break;
-		}
-
-	  }
-	if (modified_in_b)
-	  goto end_seq_and_fail;
-
-	if (!noce_emit_bb (emit_b, else_bb, b_simple))
-	  goto end_seq_and_fail;
+  if (emit_a || modified_in_a)
+    {
+      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))
+	    {
+	      modified_in_b = true;
+	      break;
+	    }
+	}
+      if (modified_in_b)
+	goto end_seq_and_fail;
 
-	if (!noce_emit_bb (emit_a, then_bb, a_simple))
-	  goto end_seq_and_fail;
-      }
-    else
-      {
-	if (!noce_emit_bb (emit_a, then_bb, a_simple))
-	  goto end_seq_and_fail;
+      if (!noce_emit_bb (emit_b, else_bb, b_simple))
+	goto end_seq_and_fail;
 
-	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
+    {
+      if (!noce_emit_bb (emit_a, then_bb, a_simple))
+	goto end_seq_and_fail;
 
-      }
+      if (!noce_emit_bb (emit_b, else_bb, b_simple))
+	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/pr68506.c b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
new file mode 100644
index 0000000000000000000000000000000000000000..15984edfe0812dc1dbbc8a07bc5b95997ed3acb9
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
@@ -0,0 +1,63 @@ 
+/* { dg-options "-fno-builtin-abort" } */
+
+int a, b, m, n, o, p, s, u, i;
+char c, q, y;
+short d;
+unsigned char e;
+static int f, h;
+static short g, r, v;
+unsigned t;
+
+extern void abort ();
+
+int
+fn1 (int p1)
+{
+  return a ? p1 : p1 + a;
+}
+
+unsigned char
+fn2 (unsigned char p1, int p2)
+{
+  return p2 >= 2 ? p1 : p1 >> p2;
+}
+
+static short
+fn3 ()
+{
+  int w, x = 0;
+  for (; p < 31; p++)
+    {
+      s = fn1 (c | ((1 && c) == c));
+      t = fn2 (s, x);
+      c = (unsigned) c > -(unsigned) ((o = (m = d = t) == p) <= 4UL) && n;
+      v = -c;
+      y = 1;
+      for (; y; y++)
+	e = v == 1;
+      d = 0;
+      for (; h != 2;)
+	{
+	  for (;;)
+	    {
+	      if (!m)
+		abort ();
+	      r = 7 - f;
+	      x = e = i | r;
+	      q = u * g;
+	      w = b == q;
+	      if (w)
+		break;
+	    }
+	  break;
+	}
+    }
+  return x;
+}
+
+int
+main ()
+{
+  fn3 ();
+  return 0;
+}