| Message ID | CACgzC7AmSV35UGQQjLbA4NLVbgk862HA9rO2KqeBzj3L_-EQMw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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, Jeff
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
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);