diff mbox

[RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves

Message ID 564DA454.2080704@arm.com
State Superseded
Headers show

Commit Message

Kyrylo Tkachov Nov. 19, 2015, 10:28 a.m. UTC
On 18/11/15 09:11, Kyrill Tkachov wrote:
>

> On 17/11/15 23:11, Bernd Schmidt wrote:

>> On 11/17/2015 02:03 PM, Kyrill Tkachov wrote:

>>> +      || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->insn))))

>>>      return false;

>>

>>> Well, I think the statement we want to make is

>>> "return false from this function if the two expressions contain the same

>>> register number".

>>

>> I looked at it again and I think I'll need more time to really figure out what's going on in this pass.

>>

>> However, about the above... either I'm very confused, or the actual statement here is "return false _unless_ the two expressions contain the same register number". In the testcase, the regs in question are ax and bx, which is then 

>> rejected if the patch has been applied.

>

> I'm sorry, it is my mistake in explaining. I meant to say:

> "return false from this function unless the two expressions contain the same

>  register number"

>

>>

>> (gdb) p tmp_reg

>> (reg:SI 0 ax)

>> (gdb) p cand->insn

>> (insn 59 117 60 7 (set (reg:SI 4 si [orig:107 D.1813 ] [107])

>>         (sign_extend:SI (reg:HI 3 bx [orig:99 D.1813 ] [99])))

>>

>> And I think it would be better to strengthen that to "return false unless registers are identical". (From the above it's clear however that a plain rtx_equal_p wouldn't work since there's an extension in the operand).

>

> So the three relevant instructions are:

> I1: (set (reg:HI 0 ax)

>      (mem:HI (symbol_ref:DI ("f"))))

>

> I2: (set (reg:SI 3 bx)

>      (if_then_else:SI (eq (reg:CCZ 17 flags)

>                 (const_int 0))

>             (reg:SI 0 ax)

>             (reg:SI 3 bx)))

>

> I3: (set (reg:SI 4 si)

>      (sign_extend:SI (reg:HI 3 bx)))

>

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

> Hope this helps.

>

>>

>> Also, I had another look at the testcase. It uses __builtin_printf and dg-output, which is at least unusual. Try to use the normal "if (cond) abort ()".

>>

>

> I did not try to remove the __builtin_printf because the use of 'f' there is needed to trigger this.

> There are at least two other dups of this PR: 68328 and 68185 the testcases for which have an "if (cond) abort ();" form.

> I'll use them instead.

>


For completeness' sake here's the patch I'm proposing.
I've used the testcases from the other two PRs that exhibit the same problem and use
the if (cond) abort (); form.

Kyrill

2015-11-16  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 defining
     insn does not feed directly into candidate extension insn.

2015-11-16  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

>>

>

Comments

Kyrylo Tkachov Nov. 20, 2015, 9:16 a.m. UTC | #1
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
diff mbox

Patch

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