This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi all,

In this PR we encounter a wrong-code bug on x86_64. It started with an RTL if-conversion patch of mine
which I believe exposed a latent issue in the ree (redundant extension elimination) pass.
The different if-conversion behaviour enabled a new cse opportunity
which then produced the RTL that triggered the bad ree behaviour.
The relevant RTL insns before the ree pass look like:

Basic Block A:
(set (reg:HI 0 ax)
     (mem:HI (symbol_ref:DI ("f"))))
...
(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)))

(set (reg:SI 4 si)
     (sign_extend:SI (reg:HI 3 bx)))
...
Basic block B (dominated by basic block A):
(set (reg:SI 4 si)
     (sign_extend:SI (reg:QI 0 ax))) /* ax contains symbol "f". */


ree changes that into the broken:
Basic block A:
(set (reg:SI 4 si)
     (sign_extend:SI (mem:HI (symbol_ref:DI ("f")))))

(set (reg:SI 3 bx)
     (reg:SI 4 si))

...
(set (reg:SI 3 bx)
     (if_then_else:SI (eq (reg:CCZ 17 flags)
                (const_int 0 [0]))
            (reg:SI 0 ax)
            (reg:SI 3 bx)))
...
Basic block B:
(set (reg:SI 4 si)
     (sign_extend:SI (reg:QI 0 ax))) /* Insn unchanged by ree, but ax now undefined.  */


Note that after ree register ax is now used uninitialised in basic block A and
the insn that previously set it to symbol "f" has now been transformed into:
(set (reg:SI 4 si)
     (sign_extend:SI (mem:HI (symbol_ref:DI ("f")))))

I've explained in the comments in the patch what's going on but the short version is
trying to change the destination of a defining insn that feeds into an extend insn
is not valid if the defining insn doesn't feed directly into the extend insn.
In the ree pass the only way this can happen is if there is an intermediate conditional
move that the pass tries to handle in a special way.  An equivalent fix would have been
to check on that path (when copy_needed in combine_reaching_defs is true) that the
state->copies_list vector (that contains the conditional move insns feeding into the
extend insn) is empty.

This patch is the minimal fix that I could do for this PR and the cases it rejects are
cases where we're pretty much guaranteed to miscompile the code, so it doesn't reject
any legitimate extension elimination opportunities. I checked that the code generation
doesn't change on aarch64 for the whole of SPEC2006 (I did see codegen regressions with
previous versions of the patch that were less targeted).

Bootstrapped and tested on x86_64-unknown-linux-gnu, arm-none-linux-gnueabihf,
aarch64-none-linux-gnu.

I've marked some other PRs that are dups of this one in the ChangeLog. If anyone
has any other PRs that are dups of this one please let me know.

Ok for trunk?

Thanks,
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.dg/pr68194.c: New test.
commit 33131f774705b936afc1a26c145e1214b388771f
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.dg/pr68194.c b/gcc/testsuite/gcc.dg/pr68194.c
new file mode 100644
index 0000000..b4855ea
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr68194.c
@@ -0,0 +1,35 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+int a, c, d, e, g, h;
+short f;
+
+short
+fn1 (void)
+{
+  int j[2];
+  for (; e; e++)
+    if (j[0])
+      for (;;)
+	;
+  if (!g)
+    return f;
+}
+
+int
+main (void)
+{
+  for (; a < 1; a++)
+    {
+      for (c = 0; c < 2; c++)
+	{
+	  d && (f = 0);
+	  h = fn1 ();
+	}
+      __builtin_printf ("%d\n", (char) f);
+   }
+
+ return 0;
+}
+
+/* { dg-output "0" } */

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]