| Message ID | 56532CE0.9050802@arm.com |
|---|---|
| State | New |
| Headers | show |
On 23/11/15 15:12, Kyrill Tkachov wrote: > Hi Bernd, > > On 20/11/15 01:41, Bernd Schmidt wrote: >>>> I1 is def_insn, I3 is cand->insn. tmp_reg is 'ax'. What we want to do >>>> is reject this transformation >>>> because the destination of def_insn (aka I1), that is 'ax', is not the >>>> operand of the extend operation >>>> in cand->insn (aka I3). As you said, rtx_equal won't work on just >>>> SET_SRC (PATTERN (cand->insn)) because >>>> it's an extend operation. So reg_overlap_mentioned should be appropriate. >> >> Yeah, so just use the src_reg variable for the comparison. I still don't see why you wouldn't want to use the stronger test. But the whole thing still feels not completely ideal somehow, so after reading through ree.c for a while and >> getting a better feeling for how it works, I think the following (which you said is equivalent) would be the most understandable and direct fix. >> >> You said that the two tests should be equivalent, and I agree. I've not found cases where the change makes a difference, other than the testcase. Would you mind running this version through the testsuite and committing if it passes? >> >> I've shrunk the comment; massive explanations like this for every bug are inappropriate IMO, and the example also duplicates an earlier comment in the same function. And, as I said earlier, the way you placed the comment is confusing >> because only one part of the following if statement is related to it. >> > > Thanks for the comments, here is the final patch that I'll be committing. > It passed testing on arm, aarch64, x86_64. > I've committed this as r230795 . This bug also affects GCC 5 and 4.9. I've confirmed that this patch fixes the miscompilations on those branches. Bootstrap and test on x86_64 on the GCC 5 branch is successful. Same on 4.9 is ongoing. The patch applies cleanly to all branches. So ok to backport to the active branches if 4.9 testing comes back clean? Thanks, Kyrill > > > 2015-11-23 Bernd Schmidt <bschmidt@redhat.com> > Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/68194 > PR rtl-optimization/68328 > PR rtl-optimization/68185 > * ree.c (combine_reaching_defs): Reject copy_needed case if > copies_list is not empty. > > 2015-11-23 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR rtl-optimization/68194 > * gcc.c-torture/execute/pr68185.c: Likewise. > * gcc.c-torture/execute/pr68328.c: Likewise. > > >> >> Bernd >
commit ceecbb45212e2c2a6650000fabba03e07f6fcbe4
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Fri Nov 13 15:01:47 2015 +0000
[RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves
diff --git a/gcc/ree.c b/gcc/ree.c
index b8436f2..9d94843 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -770,6 +770,12 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
if (state->defs_list.length () != 1)
return false;
+ /* We don't have the structure described above if there are
+ conditional moves in between the def and the candidate,
+ and we will not handle them correctly. See PR68194. */
+ if (state->copies_list.length () > 0)
+ return false;
+
/* We require the candidate not already be modified. It may,
for example have been changed from a (sign_extend (reg))
into (zero_extend (sign_extend (reg))).
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68185.c b/gcc/testsuite/gcc.c-torture/execute/pr68185.c
new file mode 100644
index 0000000..826531b
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68185.c
@@ -0,0 +1,29 @@
+int a, b, d = 1, e, f, o, u, w = 1, z;
+short c, q, t;
+
+int
+main ()
+{
+ char g;
+ for (; d; d--)
+ {
+ while (o)
+ for (; e;)
+ {
+ c = b;
+ int h = o = z;
+ for (; u;)
+ for (; a;)
+ ;
+ }
+ if (t < 1)
+ g = w;
+ f = g;
+ g && (q = 1);
+ }
+
+ if (q != 1)
+ __builtin_abort ();
+
+ return 0;
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68328.c b/gcc/testsuite/gcc.c-torture/execute/pr68328.c
new file mode 100644
index 0000000..edf244b
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68328.c
@@ -0,0 +1,44 @@
+int a, b, c = 1, d = 1, e;
+
+__attribute__ ((noinline, noclone))
+ int foo (void)
+{
+ asm volatile ("":::"memory");
+ return 4195552;
+}
+
+__attribute__ ((noinline, noclone))
+ void bar (int x, int y)
+{
+ asm volatile (""::"g" (x), "g" (y):"memory");
+ if (y == 0)
+ __builtin_abort ();
+}
+
+int
+baz (int x)
+{
+ char g, h;
+ int i, j;
+
+ foo ();
+ for (;;)
+ {
+ if (c)
+ h = d;
+ g = h < x ? h : 0;
+ i = (signed char) ((unsigned char) (g - 120) ^ 1);
+ j = i > 97;
+ if (a - j)
+ bar (0x123456, 0);
+ if (!b)
+ return e;
+ }
+}
+
+int
+main ()
+{
+ baz (2);
+ return 0;
+}