On 31 October 2013 00:08, Jeff Law <l...@redhat.com> wrote: > On 10/30/13 00:09, Zhenqiang Chen wrote: >> >> 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))) > > 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
