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]

Re: [PATCH] Keep REG_INC note in subreg2 pass


On 31 October 2013 00:08, Jeff Law <law@redhat.com> wrote:
> On 10/30/13 00:09, Zhenqiang Chen wrote:
>>
>> On 30 October 2013 02:47, Jeff Law <law@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.chen@linaro.org>
>>>>
>>>>           * lower-subreg.c (resolve_simple_move): Copy REG_INC note.
>>>>
>>>> testsuite/ChangeLog:
>>>> 2013-10-24  Zhenqiang Chen<zhenqiang.chen@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)))
>
> Formatting nit.   This should be formatted as
>
> if (MEM_P (real_dest)
>     && !(resolve_reg_p (real_dest) || resolve_subreg_p (real_dest)))
>
> If that results in too long of a line, then it should wrap like this:
>
>
> if (MEM_P (real_dest)
>     && !(resolve_reg_p (real_dest)
>          || resolve_subreg_p (real_dest)))
>
> OK with that change.  Please install on the trunk.  The 4.8 maintainers have
> the final call for the 4.8 release branch.

Thanks. Patch is committed to trunk@r204247 with the change.

-Zhenqiang


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