| Message ID | 564DA454.2080704@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
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. > Ok, thanks for the explanation. When investigating this bug I tried a patch identical to yours and it had worked just fine. My patch was just an alternative approach to the same issue. I'll retest it just to double-check and I'll incorporate the testsuite additions. Thanks for your help! Kyrill > > Bernd
commit 7ca1b135babbe3a542c850591e1b0e8736a19f55
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..e91d164 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -814,7 +814,30 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
rtx tmp_reg = gen_rtx_REG (GET_MODE (SET_DEST (PATTERN (cand->insn))),
REGNO (SET_DEST (*dest_sub_rtx)));
- if (reg_overlap_mentioned_p (tmp_reg, SET_DEST (PATTERN (cand->insn))))
+
+ /* When transforming:
+ (set (reg1) (expression))
+ ...
+ (set (reg2) (any_extend (reg1)))
+
+ into
+
+ (set (reg2) (any_extend (expression)))
+ (set (reg1) (reg2))
+ make sure that reg1 from the first set feeds directly into the extend.
+ This may not hold in a situation with an intermediate
+ conditional copy i.e.
+ I1: (set (reg3) (expression))
+ I2: (set (reg1) (cond ? reg3 : reg1))
+ I3: (set (reg2) (any_extend (reg1)))
+
+ where I3 is cand, I1 is def_insn and I2 is a conditional copy.
+ We want to avoid transforming that into:
+ (set (reg2) (any_extend (expression)))
+ (set (reg1) (reg2))
+ (set (reg1) (cond ? reg3 : reg1)). */
+ if (reg_overlap_mentioned_p (tmp_reg, SET_DEST (PATTERN (cand->insn)))
+ || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->insn))))
return false;
/* The destination register of the extension insn must not be
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;
+}