| Message ID | CACgzC7Cab2ke3L0AqGUvqdxg-PEf2r-xkwHq=OYu4UviEhyw6g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 27/06/13 10:02, Zhenqiang Chen wrote: > Hi, > > Shrink-wrap optimization sinks some instructions for more > opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB > clobbers SRC. But for ARM, gcc might generate cond_exec insns before > shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info > from cond_exec insns. So the check in function > move_insn_for_shrink_wrap is not enough. > > The patch is to add more check bases on DF_LIVE_BB_INFO (bb)->gen if > df-live is available. > > Bootstrap and no make check regression on x86-64 and Panda board. > > Is is OK for trunk? > > Thanks! > -Zhenqiang > > ChangeLog: > 2013-06-27 Zhenqiang Chen <zhenqiang.chen@linaro.org> > > PR target/57637 > * function.c (move_insn_for_shrink_wrap): Check > DF_LIVE_BB_INFO (bb)->gen. First off, this isn't really my area, so all this might be off base. Did you really mean Richard Sandiford? The code you're looking at here looks a bit odd. We have live_out = df_get_live_out (bb); live_in = df_get_live_in (next_block); bb = next_block; /* Check whether BB uses DEST or clobbers DEST. We need to add INSN to BB if so. Either way, DEST is no longer live on entry, except for any part that overlaps SRC (next loop). */ bb_uses = &DF_LR_BB_INFO (bb)->use; bb_defs = &DF_LR_BB_INFO (bb)->def; The setting of live_out and live in uses if (df_live) return DF_LIVE_OUT (bb); else return DF_LR_OUT (bb); but for bb_uses and bb_defs, we unconditionally use the LR problem and never consider live. That seems strange to me. Perhaps a better fix would be to set bb_uses and bb_defs based on whether or not df_live was valid. ie live_out = df_get_live_out (bb); live_in = df_get_live_in (next_block); bb = next_block; /* Check whether BB uses DEST or clobbers DEST. We need to add INSN to BB if so. Either way, DEST is no longer live on entry, except for any part that overlaps SRC (next loop). */ if (df_live) { bb_uses = &DF_LIVE_BB_INFO (bb)->use; bb_defs = &DF_LIVE_BB_INFO (bb)->def; } else { bb_uses = &DF_LR_BB_INFO (bb)->use; bb_defs = &DF_LR_BB_INFO (bb)->def; } Richard (S), what are your thoughts? R. > > diff --git a/gcc/function.c b/gcc/function.c > index 3e33fc7..08ca4a1 100644 > --- a/gcc/function.c > +++ b/gcc/function.c > @@ -5508,7 +5508,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, > bb_defs = &DF_LR_BB_INFO (bb)->def; > for (i = dregno; i < end_dregno; i++) > { > - if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i)) > + if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i) > + || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, i))) > next_block = NULL; > CLEAR_REGNO_REG_SET (live_out, i); > CLEAR_REGNO_REG_SET (live_in, i); > @@ -5518,7 +5519,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, > Either way, SRC is now live on entry. */ > for (i = sregno; i < end_sregno; i++) > { > - if (REGNO_REG_SET_P (bb_defs, i)) > + if (REGNO_REG_SET_P (bb_defs, i) > + || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, i))) > next_block = NULL; > SET_REGNO_REG_SET (live_out, i); > SET_REGNO_REG_SET (live_in, i); >
On 27 June 2013 21:10, Richard Earnshaw <rearnsha@arm.com> wrote: > On 27/06/13 10:02, Zhenqiang Chen wrote: >> >> Hi, >> >> Shrink-wrap optimization sinks some instructions for more >> opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB >> clobbers SRC. But for ARM, gcc might generate cond_exec insns before >> shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info >> from cond_exec insns. So the check in function >> move_insn_for_shrink_wrap is not enough. >> >> The patch is to add more check bases on DF_LIVE_BB_INFO (bb)->gen if >> df-live is available. >> >> Bootstrap and no make check regression on x86-64 and Panda board. >> >> Is is OK for trunk? >> >> Thanks! >> -Zhenqiang >> >> ChangeLog: >> 2013-06-27 Zhenqiang Chen <zhenqiang.chen@linaro.org> >> >> PR target/57637 >> * function.c (move_insn_for_shrink_wrap): Check >> DF_LIVE_BB_INFO (bb)->gen. > > > First off, this isn't really my area, so all this might be off base. Did you > really mean Richard Sandiford? > > The code you're looking at here looks a bit odd. We have > > live_out = df_get_live_out (bb); > live_in = df_get_live_in (next_block); > bb = next_block; > > /* Check whether BB uses DEST or clobbers DEST. We need to add > INSN to BB if so. Either way, DEST is no longer live on entry, > except for any part that overlaps SRC (next loop). */ > bb_uses = &DF_LR_BB_INFO (bb)->use; > > bb_defs = &DF_LR_BB_INFO (bb)->def; > > The setting of live_out and live in uses > > if (df_live) > return DF_LIVE_OUT (bb); > else > return DF_LR_OUT (bb); > > but for bb_uses and bb_defs, we unconditionally use the LR problem and never > consider live. > > That seems strange to me. > > Perhaps a better fix would be to set bb_uses and bb_defs based on whether or > not df_live was valid. ie > > live_out = df_get_live_out (bb); > live_in = df_get_live_in (next_block); > bb = next_block; > > /* Check whether BB uses DEST or clobbers DEST. We need to add > INSN to BB if so. Either way, DEST is no longer live on entry, > except for any part that overlaps SRC (next loop). */ > if (df_live) > { > bb_uses = &DF_LIVE_BB_INFO (bb)->use; > bb_defs = &DF_LIVE_BB_INFO (bb)->def; > } > else > { > bb_uses = &DF_LR_BB_INFO (bb)->use; > > bb_defs = &DF_LR_BB_INFO (bb)->def; > } Thanks for the comments. DF_LIVE_BB_INFO includes "gen" and "kill", not "def" and "use". "gen" from df_live does not cover all the information of "def" from df_lr. I had tried to set bb_defs to &DF_LIVE_BB_INFO (bb)->gen. But x86-64 bootstrap failed. -Zhenqiang >> >> diff --git a/gcc/function.c b/gcc/function.c >> index 3e33fc7..08ca4a1 100644 >> --- a/gcc/function.c >> +++ b/gcc/function.c >> @@ -5508,7 +5508,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, >> bb_defs = &DF_LR_BB_INFO (bb)->def; >> for (i = dregno; i < end_dregno; i++) >> { >> - if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, >> i)) >> + if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i) >> + || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, >> i))) >> next_block = NULL; >> CLEAR_REGNO_REG_SET (live_out, i); >> CLEAR_REGNO_REG_SET (live_in, i); >> @@ -5518,7 +5519,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, >> Either way, SRC is now live on entry. */ >> for (i = sregno; i < end_sregno; i++) >> { >> - if (REGNO_REG_SET_P (bb_defs, i)) >> + if (REGNO_REG_SET_P (bb_defs, i) >> + || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, >> i))) >> next_block = NULL; >> SET_REGNO_REG_SET (live_out, i); >> SET_REGNO_REG_SET (live_in, i); >> > >
> Shrink-wrap optimization sinks some instructions for more > opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB > clobbers SRC. But for ARM, gcc might generate cond_exec insns before > shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info > from cond_exec insns. So the check in function > move_insn_for_shrink_wrap is not enough. Posting a testcase would be even better, but the analysis looks plausible. DF_REF_CONDITIONAL and DF_REF_PARTIAL defs aren't killing defs so they are not included in the def set for LR. As you pointed out, they are included in the gen set for LIVE if it exists, so I guess we should wonder if we really want to do the scanning if LIVE doesn't exist, i.e. at -O1, instead of simply giving up. In any case, the scanning doesn't need to be redundant with the previous work, so: if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL))) continue; is missing in the patch.
On 11 July 2013 18:31, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Shrink-wrap optimization sinks some instructions for more >> opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB >> clobbers SRC. But for ARM, gcc might generate cond_exec insns before >> shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info >> from cond_exec insns. So the check in function >> move_insn_for_shrink_wrap is not enough. > > Posting a testcase would be even better, but the analysis looks plausible. > DF_REF_CONDITIONAL and DF_REF_PARTIAL defs aren't killing defs so they are not > included in the def set for LR. > > As you pointed out, they are included in the gen set for LIVE if it exists, so > I guess we should wonder if we really want to do the scanning if LIVE doesn't > exist, i.e. at -O1, instead of simply giving up. In any case, the scanning Thanks. The patch is updated based the comments: it will check GEN set of LIVE if df_live exists. Otherwise, just give up. And a testcase is included in the attached patch. -Zhenqiang > doesn't need to be redundant with the previous work, so: > > if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL))) > continue; > > is missing in the patch. > > -- > Eric Botcazou
Ping? Is the updated patch OK for trunk? Thanks! -Zhenqiang On 16 July 2013 17:29, Zhenqiang Chen <zhenqiang.chen@linaro.org> wrote: > On 11 July 2013 18:31, Eric Botcazou <ebotcazou@adacore.com> wrote: >>> Shrink-wrap optimization sinks some instructions for more >>> opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB >>> clobbers SRC. But for ARM, gcc might generate cond_exec insns before >>> shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info >>> from cond_exec insns. So the check in function >>> move_insn_for_shrink_wrap is not enough. >> >> Posting a testcase would be even better, but the analysis looks plausible. >> DF_REF_CONDITIONAL and DF_REF_PARTIAL defs aren't killing defs so they are not >> included in the def set for LR. >> >> As you pointed out, they are included in the gen set for LIVE if it exists, so >> I guess we should wonder if we really want to do the scanning if LIVE doesn't >> exist, i.e. at -O1, instead of simply giving up. In any case, the scanning > > Thanks. > > The patch is updated based the comments: it will check GEN set of LIVE > if df_live exists. Otherwise, just give up. > > And a testcase is included in the attached patch. > > -Zhenqiang > > >> doesn't need to be redundant with the previous work, so: >> >> if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL))) >> continue; >> >> is missing in the patch. >> >> -- >> Eric Botcazou
> The patch is updated based the comments: it will check GEN set of LIVE > if df_live exists. Otherwise, just give up. The patch is missing a ChangeLog. Otherwise it looks good, modulo: + /* DF_LR_BB_INFO (bb)->def does not kill the DF_REF_PARTIAL and + DF_REF_CONDITIONAL defs. So if DF_LIVE doesn't exist, i.e. "does not comprise" + at -O1, just give up to search NEXT_BLOCK "just give up searching" I'm not sure this matters in practice, but don't we need to set the appropriate bit in the GEN set of BB at the end of the function if df_live is non-zero, at least for the sake of consistency? > And a testcase is included in the attached patch. Thanks. We generally try to make the testcases self-contained, i.e. remove the #include's as much as possible. It seems that stdlib.h is included for abort; if so, remove it and use __builtin_abort instead. Would it be possible to remove stdio.h as well?
On 22 July 2013 17:56, Eric Botcazou <ebotcazou@adacore.com> wrote: >> The patch is updated based the comments: it will check GEN set of LIVE >> if df_live exists. Otherwise, just give up. > > The patch is missing a ChangeLog. Otherwise it looks good, modulo: > > + /* DF_LR_BB_INFO (bb)->def does not kill the DF_REF_PARTIAL and > + DF_REF_CONDITIONAL defs. So if DF_LIVE doesn't exist, i.e. > > "does not comprise" > > + at -O1, just give up to search NEXT_BLOCK > > "just give up searching" Updated. > I'm not sure this matters in practice, but don't we need to set the appropriate > bit in the GEN set of BB at the end of the function if df_live is non-zero, at > least for the sake of consistency? And if df_live is non-zero, do we need update df_lr's IN and OUT? I think we need another patch to make all these consistency. >> And a testcase is included in the attached patch. > > Thanks. We generally try to make the testcases self-contained, i.e. remove the > #include's as much as possible. It seems that stdlib.h is included for abort; > if so, remove it and use __builtin_abort instead. Would it be possible to > remove stdio.h as well? Update abort to __builtin_abort and malloc to __builtin_malloc. And remove all the include. ChangeLog 2013-07-22 Zhenqiang Chen <zhenqiang.chen@linaro.org> * function.c (move_insn_for_shrink_wrap): check gen of df_live if it exists, otherwise (-O1) give up searching. gcc/testsuite/ChangeLog 2013-07-22 Zhenqiang Chen <zhenqiang.chen@linaro.org> * gcc.target/arm/pr57637.c: New added. Is it OK? Thanks! -Zhenqiang
> And if df_live is non-zero, do we need update df_lr's IN and OUT? I think > we need another patch to make all these consistency. Possibly, but this would belong to another patch. I nevertheless think that we should set the bit in the GEN set because we'll be testing the GEN set now. The patch is OK with this change if it passes the usual testing. > ChangeLog > 2013-07-22 Zhenqiang Chen <zhenqiang.chen@linaro.org> > > * function.c (move_insn_for_shrink_wrap): check gen of df_live if > it exists, otherwise (-O1) give up searching. Capital letter at the beginning, and I'd expand a little on it, something like: * function.c (move_insn_for_shrink_wrap): Also check the GEN set of the LIVE problem for the liveness analysis if it exists, otherwise give up. > gcc/testsuite/ChangeLog > 2013-07-22 Zhenqiang Chen <zhenqiang.chen@linaro.org> > > * gcc.target/arm/pr57637.c: New added. "New testcase"
On 22 July 2013 21:16, Eric Botcazou <ebotcazou@adacore.com> wrote: >> And if df_live is non-zero, do we need update df_lr's IN and OUT? I think >> we need another patch to make all these consistency. > > Possibly, but this would belong to another patch. I nevertheless think that we > should set the bit in the GEN set because we'll be testing the GEN set now. > > The patch is OK with this change if it passes the usual testing. Thanks. Bootstrap on x86-64, ARM chrome book and Pandaboard. No make check regression on x86-64 and Pandaboard. The patch is committed with the ChangeLog updated according to your comments. -Zhenqiang >> ChangeLog >> 2013-07-22 Zhenqiang Chen <zhenqiang.chen@linaro.org> >> >> * function.c (move_insn_for_shrink_wrap): check gen of df_live if >> it exists, otherwise (-O1) give up searching. > > Capital letter at the beginning, and I'd expand a little on it, something like: > > * function.c (move_insn_for_shrink_wrap): Also check the GEN set of the > LIVE problem for the liveness analysis if it exists, otherwise give up. > >> gcc/testsuite/ChangeLog >> 2013-07-22 Zhenqiang Chen <zhenqiang.chen@linaro.org> >> >> * gcc.target/arm/pr57637.c: New added. > > "New testcase" > > -- > Eric Botcazou
diff --git a/gcc/function.c b/gcc/function.c index 3e33fc7..08ca4a1 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -5508,7 +5508,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, bb_defs = &DF_LR_BB_INFO (bb)->def; for (i = dregno; i < end_dregno; i++) { - if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i)) + if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i) + || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, i))) next_block = NULL; CLEAR_REGNO_REG_SET (live_out, i); CLEAR_REGNO_REG_SET (live_in, i); @@ -5518,7 +5519,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn, Either way, SRC is now live on entry. */ for (i = sregno; i < end_sregno; i++) { - if (REGNO_REG_SET_P (bb_defs, i)) + if (REGNO_REG_SET_P (bb_defs, i) + || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, i))) next_block = NULL; SET_REGNO_REG_SET (live_out, i); SET_REGNO_REG_SET (live_in, i);
Hi, Shrink-wrap optimization sinks some instructions for more opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB clobbers SRC. But for ARM, gcc might generate cond_exec insns before shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info from cond_exec insns. So the check in function move_insn_for_shrink_wrap is not enough. The patch is to add more check bases on DF_LIVE_BB_INFO (bb)->gen if df-live is available. Bootstrap and no make check regression on x86-64 and Panda board. Is is OK for trunk? Thanks! -Zhenqiang ChangeLog: 2013-06-27 Zhenqiang Chen <zhenqiang.chen@linaro.org> PR target/57637 * function.c (move_insn_for_shrink_wrap): Check DF_LIVE_BB_INFO (bb)->gen.