diff mbox

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

Message ID 5656E924.4030603@arm.com
State Superseded
Headers show

Commit Message

Kyrylo Tkachov Nov. 26, 2015, 11:12 a.m. UTC
Hi all,

In this PR we have an IF-THEN-JOIN formation i.e. no ELSE block and we have a situation
where the THEN block modifies a register used in emit_b, so emit_b must be emitted before
the THEN block. However the bug in the logic that performs these checks ends up to us
emitting emit_a+then_bb followed by emit_b+else_bb.

The fix is pretty simple and involves emitting emit_b (+ else_bb that is empty in this case)
if modified_a is true, even if emit_a is NULL. If emit_a is NULL noce_emit_bb will handle
it properly and not do anything bad, so we're safe.

Bootstrapped and tested on arm, aarch64, x86_64.

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'.

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

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

Comments

Kyrylo Tkachov Nov. 26, 2015, 1:52 p.m. UTC | #1
On 26/11/15 13:40, Bernd Schmidt wrote:
> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:

>> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c

>> index af7a3b9..3e3dc8d 100644

>> --- a/gcc/ifcvt.c

>> +++ b/gcc/ifcvt.c

>> @@ -2220,7 +2220,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)

>>         }

>>

>>       }

>> -    if (emit_a && modified_in_a)

>> +    if (emit_a || modified_in_a)

>>         {

> Having stared at it in the debugger for a while, I think I managed to convince myself that this is correct. So, OK.

>


Thanks.

> A few other comments. This whole if block is indented too far, please fix while you're there. Also eliminate the unnecessary blank lines before closing braces (two instances inside this if block). There are other formatting errors in this 

> function, but those are best left alone for now.


Ok, I'll fix the indentation in that if-else block

>

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

Thanks,
Kyrill
>

> Bernd

>
Kyrylo Tkachov Nov. 26, 2015, 2:35 p.m. UTC | #2
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.

> 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

>
diff mbox

Patch

commit 08a371d20793bd57e9a68b85beaf2cab0804ed48
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Nov 24 11:49:30 2015 +0000

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

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index af7a3b9..3e3dc8d 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2220,7 +2220,7 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
 	  }
 
     }
-    if (emit_a && modified_in_a)
+    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)
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 0000000..15984ed
--- /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;
+}