Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law <law at redhat dot com>]

On 01/10/14 14:52, Jakub Jelinek wrote:
> There is one thing I still worry about, if some target has
> an insn to say sign extend or zero extend a short memory load
> into HARD_REGNO_NREGS () > 1 register, but the other involved
> register has the only one (or fewer) hard registers available to it.
> Consider registers SImode hard registers 0, 1, 2, 3:
>    (set (reg:SI 3) (something:SI))
>    (set (reg:HI 0) (expression:HI))
>    (set (reg:SI 2) (sign_extend:SI (reg:HI 0)))
>    (set (reg:DI 0) (sign_extend:DI (reg:HI 0)))
>    (use (reg:SI 3))
> we transform this into:
>    (set (reg:SI 3) (something:SI))
>    (set (reg:SI 2) (sign_extend:SI (expression:HI)))
>    (set (reg:SI 0) (reg:HI 2))
>    (set (reg:DI 0) (sign_extend:DI (reg:HI 0)))
>    (use (reg:SI 3))
> first (well, the middle is then pending in copy list), and next:
>    (set (reg:SI 3) (something))
>    (set (reg:DI 2) (sign_extend:DI (expression:HI)))
>    (set (reg:DI 0) (reg:DI 2))
>    (use (reg:SI 3))
> but that looks wrong, because the second instruction would now clobber
> (reg:SI 3).  Dunno if we have such an target and thus if it is possible
> to construct a testcase.
No need to construct a testcase, there's a few that trip the condition 
in the existing testsuite :-)

Basically I just put a check in combine_set_extension to detect when 
widening of the result of the reaching def requires more hard registers 
than it previously needed and ran the testsuite.



> So, I'd say the handling of the second extend should notice that
> it is actually extending load into a different register and bail out
> if it would need more hard registers than it needed previously, or
> something similar.
Yes, like in the attached patch?  OK for the trunk?


commit 1313449102ac8d62e36818d8660ef2e897bd59e3
Author: Jeff Law <law@redhat.com>
Date:   Fri Jan 10 14:31:15 2014 -0700

    	PR tree-optimization/59747
    	* ree.c (find_and_remove_re): Properly handle case where a second
    	eliminated extension requires widening a copy created for elimination
    	of a prior extension.
    	(combine_set_extension): Ensure that the number of hard regs needed
    	for a destination register does not change when we widen it.
    
    	PR tree-optimization/59747
    	* gcc.c-torture/execute/pr59747.c: New test.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index c554609..a82e23c 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -5,6 +5,13 @@
 	occurs before the extension when optimizing extensions with
 	different source and destination hard registers.
 
+	PR tree-optimization/59747
+	* ree.c (find_and_remove_re): Properly handle case where a second
+	eliminated extension requires widening a copy created for elimination
+	of a prior extension.
+	(combine_set_extension): Ensure that the number of hard regs needed
+	for a destination register does not change when we widen it.
+
 2014-01-10  Jan Hubicka  <jh@suse.cz>
 
 	PR ipa/58585
diff --git a/gcc/ree.c b/gcc/ree.c
index 63cc8cc..3ee97cd 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -297,6 +297,13 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx *orig_set)
   else
     new_reg = gen_rtx_REG (cand->mode, REGNO (SET_DEST (*orig_set)));
 
+  /* We're going to be widening the result of DEF_INSN, ensure that doing so
+     doesn't change the number of hard registers needed for the result.  */
+  if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
+      != HARD_REGNO_NREGS (REGNO (SET_SRC (*orig_set)),
+			   GET_MODE (SET_DEST (*orig_set))))
+	return false;
+
   /* Merge constants by directly moving the constant into the register under
      some conditions.  Recall that RTL constants are sign-extended.  */
   if (GET_CODE (orig_src) == CONST_INT
@@ -1017,11 +1024,20 @@ find_and_remove_re (void)
   for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
     {
       rtx curr_insn = reinsn_copy_list[i];
+      rtx def_insn = reinsn_copy_list[i + 1];
+
+      /* Use the mode of the destination of the defining insn
+	 for the mode of the copy.  This is necessary if the
+	 defining insn was used to eliminate a second extension
+	 that was wider than the first.  */
+      rtx sub_rtx = *get_sub_rtx (def_insn);
       rtx pat = PATTERN (curr_insn);
-      rtx new_reg = gen_rtx_REG (GET_MODE (SET_DEST (pat)),
+      rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
 				 REGNO (XEXP (SET_SRC (pat), 0)));
-      rtx set = gen_rtx_SET (VOIDmode, new_reg, SET_DEST (pat));
-      emit_insn_after (set, reinsn_copy_list[i + 1]);
+      rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
+				 REGNO (SET_DEST (pat)));
+      rtx set = gen_rtx_SET (VOIDmode, new_dst, new_src);
+      emit_insn_after (set, def_insn);
     }
 
   /* Delete all useless extensions here in one sweep.  */
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index f40d56e..a603952 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -3,6 +3,9 @@
 	PR middle-end/59743
 	* gcc.c-torture/compile/pr59743.c: New test.
 
+	PR tree-optimization/59747
+	* gcc.c-torture/execute/pr59747.c: New test.
+
 2014-01-10  Jan Hubicka  <jh@suse.cz>
 
 	PR ipa/58585
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr59747.c b/gcc/testsuite/gcc.c-torture/execute/pr59747.c
new file mode 100644
index 0000000..d45a908
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr59747.c
@@ -0,0 +1,27 @@
+extern void abort (void);
+extern void exit (int);
+
+int a[6], b, c = 1, d;
+short e;
+
+int __attribute__ ((noinline))
+fn1 (int p)
+{
+  b = a[p];
+}
+
+int
+main ()
+{
+  if (sizeof (long long) != 8)
+    exit (0);
+
+  a[0] = 1;
+  if (c)
+    e--;
+  d = e;
+  long long f = e;
+  if (fn1 ((f >> 56) & 1) != 0)
+    abort ();
+  exit (0);
+}