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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

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

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