Re: [PATCH] Keep REG_INC note in subreg2 pass

[Zhenqiang Chen]

On 30 October 2013 02:47, Jeff Law <l...@redhat.com> wrote:
> On 10/24/13 02:20, Zhenqiang Chen wrote:
>>
>> Hi,
>>
>> REG_INC note is lost in subreg2 pass when resolve_simple_move, which
>> might lead to wrong dependence for ira. e.g. In function
>> validate_equiv_mem of ira.c, it checks REG_INC note:
>>
>>       for (note = REG_NOTES (insn); note; note = XEXP (note, 1))
>>          if ((REG_NOTE_KIND (note) == REG_INC
>>               || REG_NOTE_KIND (note) == REG_DEAD)
>>              && REG_P (XEXP (note, 0))
>>              && reg_overlap_mentioned_p (XEXP (note, 0), memref))
>>            return 0;
>>
>> Without REG_INC note, validate_equiv_mem will return a wrong result.
>>
>> Referhttps://bugs.launchpad.net/gcc-linaro/+bug/1243022  for more
>>
>> detail about a real case in kernel.
>>
>> Bootstrap and no make check regression on X86-64 and ARM.
>>
>> Is it OK for trunk and 4.8?
>>
>> Thanks!
>> -Zhenqiang
>>
>> ChangeLog:
>> 2013-10-24  Zhenqiang Chen<zhenqiang.c...@linaro.org>
>>
>>          * lower-subreg.c (resolve_simple_move): Copy REG_INC note.
>>
>> testsuite/ChangeLog:
>> 2013-10-24  Zhenqiang Chen<zhenqiang.c...@linaro.org>
>>
>>          * gcc.target/arm/lp1243022.c: New test.
>
> This clearly handles adding a note when the destination is a MEM with a side
> effect.  What about cases where the side effect is associated with a load
> from memory rather than a store to memory?

Yes. We should handle load from memory.

>>
>>
>> lp1243022.patch
>>
>>
>> diff --git a/gcc/lower-subreg.c b/gcc/lower-subreg.c
>> index 57b4b3c..e710fa5 100644
>> --- a/gcc/lower-subreg.c
>> +++ b/gcc/lower-subreg.c
>> @@ -1056,6 +1056,22 @@ resolve_simple_move (rtx set, rtx insn)
>>         mdest = simplify_gen_subreg (orig_mode, dest, GET_MODE (dest), 0);
>>         minsn = emit_move_insn (real_dest, mdest);
>>
>> +#ifdef AUTO_INC_DEC
>> +      /* Copy the REG_INC notes.  */
>> +      if (MEM_P (real_dest) && !(resolve_reg_p (real_dest)
>> +                                || resolve_subreg_p (real_dest)))
>> +       {
>> +         rtx note = find_reg_note (insn, REG_INC, NULL_RTX);
>> +         if (note)
>> +           {
>> +             if (!REG_NOTES (minsn))
>> +               REG_NOTES (minsn) = note;
>> +             else
>> +               add_reg_note (minsn, REG_INC, note);
>> +           }
>> +       }
>> +#endif
>
> If MINSN does not have any notes, then this results in MINSN and INSN
> sharing the note.  Note carefully that notes are chained (see implementation
> of add_reg_note).  Thus the sharing would result in MINSN and INSN actually
> sharing a chain of notes.  I'm pretty sure that's not what you intended.  I
> think you need to always use add_reg_note.

Yes. I should use add_reg_note.

Here is the updated patch:

diff --git a/gcc/lower-subreg.c b/gcc/lower-subreg.c
index ebf364f..16dfa62 100644
--- a/gcc/lower-subreg.c
+++ b/gcc/lower-subreg.c
@@ -967,7 +967,20 @@ resolve_simple_move (rtx set, rtx insn)
       rtx reg;

       reg = gen_reg_rtx (orig_mode);
+
+#ifdef AUTO_INC_DEC
+      {
+       rtx move = emit_move_insn (reg, src);
+       if (MEM_P (src))
+         {
+           rtx note = find_reg_note (insn, REG_INC, NULL_RTX);
+           if (note)
+             add_reg_note (move, REG_INC, XEXP (note, 0));
+         }
+      }
+#else
       emit_move_insn (reg, src);
+#endif
       src = reg;
     }

@@ -1057,6 +1070,16 @@ resolve_simple_move (rtx set, rtx insn)
        mdest = simplify_gen_subreg (orig_mode, dest, GET_MODE (dest), 0);
       minsn = emit_move_insn (real_dest, mdest);

+#ifdef AUTO_INC_DEC
+  if (MEM_P (real_dest) && !(resolve_reg_p (real_dest)
+                            || resolve_subreg_p (real_dest)))
+    {
+      rtx note = find_reg_note (insn, REG_INC, NULL_RTX);
+      if (note)
+       add_reg_note (minsn, REG_INC, XEXP (note, 0));
+    }
+#endif
+
       smove = single_set (minsn);
       gcc_assert (smove != NULL_RTX);