| Message ID | CAGWvnyn9J43o0oQc7kwM87zFzyY+85q9WoZgh=yX=WVsye9Luw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 30, 2017 at 2:36 PM, David Edelsohn <dje.gcc@gmail.com> wrote: > Convert the rs6000 port to use the new API for branch probabilities. > > Okay? > > Thanks, David > > * config/rs6000/rs6000.c (emit_unlikely_jump): Adjust to new branch > probability data type. > > Index: rs6000.c > =================================================================== > --- rs6000.c (revision 249839) > +++ rs6000.c (working copy) > @@ -23514,10 +23514,9 @@ > static void > emit_unlikely_jump (rtx cond, rtx label) > { > - int very_unlikely = REG_BR_PROB_BASE / 100 - 1; > rtx x = gen_rtx_IF_THEN_ELSE (VOIDmode, cond, label, pc_rtx); > rtx_insn *insn = emit_jump_insn (gen_rtx_SET (pc_rtx, x)); > - add_int_reg_note (insn, REG_BR_PROB, very_unlikely); > + add_int_reg_note (insn, REG_BR_PROB, profile_probability::very_unlikely ()); Hmmm isn't this very unlikely to work :) ? I used this as inspiration to do this for the arm ports but add_int_reg_note expects an integer but very_unlikely returns profile_probability ... regards Ramana Ramana > } > > /* A subroutine of the atomic operation splitters. Emit a load-locked
On Fri, Jun 30, 2017 at 10:36:27PM +0100, Ramana Radhakrishnan wrote: > On Fri, Jun 30, 2017 at 2:36 PM, David Edelsohn <dje.gcc@gmail.com> wrote: > > Convert the rs6000 port to use the new API for branch probabilities. > > > > Okay? > > > > Thanks, David > > > > * config/rs6000/rs6000.c (emit_unlikely_jump): Adjust to new branch > > probability data type. > > > > Index: rs6000.c > > =================================================================== > > --- rs6000.c (revision 249839) > > +++ rs6000.c (working copy) > > @@ -23514,10 +23514,9 @@ > > static void > > emit_unlikely_jump (rtx cond, rtx label) > > { > > - int very_unlikely = REG_BR_PROB_BASE / 100 - 1; > > rtx x = gen_rtx_IF_THEN_ELSE (VOIDmode, cond, label, pc_rtx); > > rtx_insn *insn = emit_jump_insn (gen_rtx_SET (pc_rtx, x)); > > - add_int_reg_note (insn, REG_BR_PROB, very_unlikely); > > + add_int_reg_note (insn, REG_BR_PROB, profile_probability::very_unlikely ()); > > Hmmm isn't this very unlikely to work :) ? > > I used this as inspiration to do this for the arm ports but > add_int_reg_note expects an integer but very_unlikely returns > profile_probability ... It probably should be converted using to_reg_br_prob_base ? Segher
On Fri, Jun 30, 2017 at 7:18 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Fri, Jun 30, 2017 at 10:36:27PM +0100, Ramana Radhakrishnan wrote: >> On Fri, Jun 30, 2017 at 2:36 PM, David Edelsohn <dje.gcc@gmail.com> wrote: >> > Convert the rs6000 port to use the new API for branch probabilities. >> > >> > Okay? >> > >> > Thanks, David >> > >> > * config/rs6000/rs6000.c (emit_unlikely_jump): Adjust to new branch >> > probability data type. >> > >> > Index: rs6000.c >> > =================================================================== >> > --- rs6000.c (revision 249839) >> > +++ rs6000.c (working copy) >> > @@ -23514,10 +23514,9 @@ >> > static void >> > emit_unlikely_jump (rtx cond, rtx label) >> > { >> > - int very_unlikely = REG_BR_PROB_BASE / 100 - 1; >> > rtx x = gen_rtx_IF_THEN_ELSE (VOIDmode, cond, label, pc_rtx); >> > rtx_insn *insn = emit_jump_insn (gen_rtx_SET (pc_rtx, x)); >> > - add_int_reg_note (insn, REG_BR_PROB, very_unlikely); >> > + add_int_reg_note (insn, REG_BR_PROB, profile_probability::very_unlikely ()); >> >> Hmmm isn't this very unlikely to work :) ? >> >> I used this as inspiration to do this for the arm ports but >> add_int_reg_note expects an integer but very_unlikely returns >> profile_probability ... > > It probably should be converted using to_reg_br_prob_base ? The comments in profile-count.h state that this should go away. We need advice from Honza about the preferred way to transform these idioms. Thanks, David
> >> > * config/rs6000/rs6000.c (emit_unlikely_jump): Adjust to new branch > >> > probability data type. > >> > > >> > Index: rs6000.c > >> > =================================================================== > >> > --- rs6000.c (revision 249839) > >> > +++ rs6000.c (working copy) > >> > @@ -23514,10 +23514,9 @@ > >> > static void > >> > emit_unlikely_jump (rtx cond, rtx label) > >> > { > >> > - int very_unlikely = REG_BR_PROB_BASE / 100 - 1; > >> > rtx x = gen_rtx_IF_THEN_ELSE (VOIDmode, cond, label, pc_rtx); > >> > rtx_insn *insn = emit_jump_insn (gen_rtx_SET (pc_rtx, x)); > >> > - add_int_reg_note (insn, REG_BR_PROB, very_unlikely); > >> > + add_int_reg_note (insn, REG_BR_PROB, profile_probability::very_unlikely ()); > >> > >> Hmmm isn't this very unlikely to work :) ? > >> > >> I used this as inspiration to do this for the arm ports but > >> add_int_reg_note expects an integer but very_unlikely returns > >> profile_probability ... > > > > It probably should be converted using to_reg_br_prob_base ? > > The comments in profile-count.h state that this should go away. > > We need advice from Honza about the preferred way to transform these idioms. I plan to change REG_BR_PROB notes to preserve all information from profile_probability (this is needed to make RTL expansion splitting work as expected), but for now they are still just REG_BR_PROB_BASE fixpoint. I think the code can stay as it is. I will add APIs for emitting/interpretting br_prob_nodes as followup (after debugging fixing issues with profile updating which I can now detect with the new type) Thanks for looking into this. Honza > > Thanks, David
On Sat, Jul 1, 2017 at 9:06 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >> >> > * config/rs6000/rs6000.c (emit_unlikely_jump): Adjust to new branch >> >> > probability data type. >> >> > >> >> > Index: rs6000.c >> >> > =================================================================== >> >> > --- rs6000.c (revision 249839) >> >> > +++ rs6000.c (working copy) >> >> > @@ -23514,10 +23514,9 @@ >> >> > static void >> >> > emit_unlikely_jump (rtx cond, rtx label) >> >> > { >> >> > - int very_unlikely = REG_BR_PROB_BASE / 100 - 1; >> >> > rtx x = gen_rtx_IF_THEN_ELSE (VOIDmode, cond, label, pc_rtx); >> >> > rtx_insn *insn = emit_jump_insn (gen_rtx_SET (pc_rtx, x)); >> >> > - add_int_reg_note (insn, REG_BR_PROB, very_unlikely); >> >> > + add_int_reg_note (insn, REG_BR_PROB, profile_probability::very_unlikely ()); >> >> >> >> Hmmm isn't this very unlikely to work :) ? >> >> >> >> I used this as inspiration to do this for the arm ports but >> >> add_int_reg_note expects an integer but very_unlikely returns >> >> profile_probability ... >> > >> > It probably should be converted using to_reg_br_prob_base ? >> >> The comments in profile-count.h state that this should go away. >> >> We need advice from Honza about the preferred way to transform these idioms. > > I plan to change REG_BR_PROB notes to preserve all information from > profile_probability (this is needed to make RTL expansion splitting work as > expected), but for now they are still just REG_BR_PROB_BASE fixpoint. > > I think the code can stay as it is. I will add APIs for emitting/interpretting > br_prob_nodes as followup (after debugging fixing issues with profile updating > which I can now detect with the new type) > > Thanks for looking into this. Does the computed value of very_unlikely need to change for the new scale? Can the profile machinery provide a helper function or macro instead of the current calculation replicated in many ports? Thanks, David
> On Sat, Jul 1, 2017 at 9:06 AM, Jan Hubicka <hubicka@ucw.cz> wrote: > >> >> > * config/rs6000/rs6000.c (emit_unlikely_jump): Adjust to new branch > >> >> > probability data type. > >> >> > > >> >> > Index: rs6000.c > >> >> > =================================================================== > >> >> > --- rs6000.c (revision 249839) > >> >> > +++ rs6000.c (working copy) > >> >> > @@ -23514,10 +23514,9 @@ > >> >> > static void > >> >> > emit_unlikely_jump (rtx cond, rtx label) > >> >> > { > >> >> > - int very_unlikely = REG_BR_PROB_BASE / 100 - 1; > >> >> > rtx x = gen_rtx_IF_THEN_ELSE (VOIDmode, cond, label, pc_rtx); > >> >> > rtx_insn *insn = emit_jump_insn (gen_rtx_SET (pc_rtx, x)); > >> >> > - add_int_reg_note (insn, REG_BR_PROB, very_unlikely); > >> >> > + add_int_reg_note (insn, REG_BR_PROB, profile_probability::very_unlikely ()); > >> >> > >> >> Hmmm isn't this very unlikely to work :) ? > >> >> > >> >> I used this as inspiration to do this for the arm ports but > >> >> add_int_reg_note expects an integer but very_unlikely returns > >> >> profile_probability ... > >> > > >> > It probably should be converted using to_reg_br_prob_base ? > >> > >> The comments in profile-count.h state that this should go away. > >> > >> We need advice from Honza about the preferred way to transform these idioms. > > > > I plan to change REG_BR_PROB notes to preserve all information from > > profile_probability (this is needed to make RTL expansion splitting work as > > expected), but for now they are still just REG_BR_PROB_BASE fixpoint. > > > > I think the code can stay as it is. I will add APIs for emitting/interpretting > > br_prob_nodes as followup (after debugging fixing issues with profile updating > > which I can now detect with the new type) > > > > Thanks for looking into this. > > Does the computed value of very_unlikely need to change for the new > scale? Can the profile machinery provide a helper function or macro > instead of the current calculation replicated in many ports? There is PROB_VERY_UNLIKELY macro which should be used in this context. Not sure how and whhen this very_unlikely got in. It is defined as (REG_BR_PROB_BASE / 2000 - 1) perhaps 2000 was consider just too strong here? Honza > > Thanks, David
> Does the computed value of very_unlikely need to change for the new > scale? Can the profile machinery provide a helper function or macro > instead of the current calculation replicated in many ports? And to answer your first question, there is REG_BR_PROB_BASE scale which is not chnaged by my patch and there is also internal scale for profile_probability (which is completely captured by the type) which currently is also set to 10000 to let me compare profiles produced before and after the conversion. I plan to set it to more sane value soon after I am reasonably sure that there is nothing wrong and we don't have code quality regressions related to the revamp. General plan is: 1) fix issues with updating and add verifier that when profile is available all probabilities and counts are initialized 2) eliminate REG_BR_PROB_BASE uses where it is not necessary (we use it for various internal scaling purposes, i.e. when verioning loops) 3) work on removing frequencies (in favour of counts) for callgraph. Because counts have quality now, it is easy to declare that given count was determined by static profile estimation and is function local. 4) remove frequencies from CFG 5) remove edge counts from CFG and keep only probabilities. For this probabilities will need to get more precise (by increasing the scale) 6) work on elimination of REG_BR_PROB_BASE representation from RTL Honza
Index: rs6000.c =================================================================== --- rs6000.c (revision 249839) +++ rs6000.c (working copy) @@ -23514,10 +23514,9 @@ static void emit_unlikely_jump (rtx cond, rtx label) { - int very_unlikely = REG_BR_PROB_BASE / 100 - 1; rtx x = gen_rtx_IF_THEN_ELSE (VOIDmode, cond, label, pc_rtx); rtx_insn *insn = emit_jump_insn (gen_rtx_SET (pc_rtx, x)); - add_int_reg_note (insn, REG_BR_PROB, very_unlikely); + add_int_reg_note (insn, REG_BR_PROB, profile_probability::very_unlikely ()); } /* A subroutine of the atomic operation splitters. Emit a load-locked