On Wed, Aug 27, 2014 at 6:34 PM, Richard Earnshaw <rearn...@arm.com> wrote:
> On 27/08/14 11:08, Bin Cheng wrote:
>> Hi
>> As reported in bug pr62151, combine pass may wrongly delete necessary
>> instruction in function distribute_notes thus leaving register
>> uninitialized. This patch is to fix the issue by checking if i2 immediately
>> modifies the register in REG_DEAD note. If yes, set tem_insn accordingly to
>> start finding right place for note distribution from i2.
>>
>> I once sent a RFC patch at
>> https://gcc.gnu.org/ml/gcc-patches/2014-08/msg01718.html, but got no
>> comments, here I added some comments in this patch to make it a formal one.
>>
>>
>> I tested the original patch, and will re-test it against the latest code
>> later. So is it OK? Any comments will be appreciated.
>>
>
> Isn't this the sort of sequence that combinable_i3pat is supposed to reject?
Hi Richard,
I think it's not. combinable_i3pat checks cases in which i1 feeds to
i3 directly, while i2 kills reg_use in i1. For this case the feeding
chain is "i0->i1->i2->i3", the combination is valid and beneficial.
What needs to be handled is if i2dest is anticipated after i3. If
not, then i2 could be deleted; if yes, i2 should be preserved.
Moreover, following constant propagation could delete i2 after
propagating the new i2src into i4. Note combine pass already handles
this kind of case using variable added_sets_2 in function try_combine.
The problem is in distribute_notes which mis-deleted i2.
I added one test case in the updated patch.
Thanks,
bin
>
Index: gcc/testsuite/gcc.c-torture/execute/pr62151.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/pr62151.c (revision 0)
+++ gcc/testsuite/gcc.c-torture/execute/pr62151.c (revision 0)
@@ -0,0 +1,41 @@
+/* PR rtl-optimization/62151 */
+
+int a, c, d, e, f, g, h, i;
+short b;
+
+int
+fn1 ()
+{
+ b = 0;
+ for (;;)
+ {
+ int j[2];
+ j[f] = 0;
+ if (h)
+ d = 0;
+ else
+ {
+ for (; f; f++)
+ ;
+ for (a = 0; a < 1; a++)
+ for (;;)
+ {
+ i = b & ((b ^ 1) & 83647) ? b : b - 1;
+ g = 1 ? i : 0;
+ e = j[0];
+ if (c)
+ break;
+ return 0;
+ }
+ }
+ }
+}
+
+int
+main ()
+{
+ fn1 ();
+ if (g != -1)
+ __builtin_abort ();
+ return 0;
+}
Index: gcc/combine.c
===================================================================
--- gcc/combine.c (revision 214413)
+++ gcc/combine.c (working copy)
@@ -13499,7 +13499,38 @@ distribute_notes (rtx notes, rtx_insn *from_insn,
|| rtx_equal_p (XEXP (note, 0), elim_i1)
|| rtx_equal_p (XEXP (note, 0), elim_i0))
break;
- tem_insn = i3;
+
+ /* See PR62151.
+ It's possible to have below situation:
+ i0: r1 <- const_0
+ i1: r2 <- r1 op_1 const_1
+ REG_DEAD r1
+ i2: r1 <- r2 op_2 const_2
+ REG_DEAD r2
+ i3: r3 <- r1
+ i4: r4 <- r1
+
+ It is transformed into below code before distributing
+ the REG_DEAD note in i1:
+ i0: NOTE_INSN_DELETED
+ i1: NOTE_INSN_DELETED
+ i2: r1 <- const_combined
+ i3: r3 <- const_combined
+ i4: r4 <- r1
+
+ We need to check if i2 immediately modifies r1 otherwise
+ i2 would be deleted by below code when distributing
+ REG_DEAD note, leaving r1 in i4 uninitialied.
+
+ We set TEM_INSN to i2 for this case indicating that we
+ need to find right place for distribution from i2.
+ */
+ if (from_insn && i2
+ && from_insn != i2 && from_insn != i3
+ && reg_set_p (XEXP (note, 0), PATTERN (i2)))
+ tem_insn = i2;
+ else
+ tem_insn = i3;
}
if (place == 0)
