| Message ID | 5656E924.4030603@arm.com |
|---|---|
| State | New |
| Headers | show |
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. 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. > 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? Bernd
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 >
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. 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. Bernd
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 >
On 11/26/2015 03:35 PM, Kyrill Tkachov wrote: > 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. Sure. Bernd
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;
+}
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.