[PATCH] rs6000 branch probability changes

[PATCH] rs6000 branch probability changes

[David Edelsohn]

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 ());
 }

 /* A subroutine of the atomic operation splitters.  Emit a load-locked

Re: [PATCH] rs6000 branch probability changes

[Ramana Radhakrishnan]

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

Re: [PATCH] rs6000 branch probability changes

[Segher Boessenkool]

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

Re: [PATCH] rs6000 branch probability changes

[David Edelsohn]

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

Re: [PATCH] rs6000 branch probability changes

[Jan Hubicka]

> >> > * 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

Re: [PATCH] rs6000 branch probability changes

[David Edelsohn]

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

Re: [PATCH] rs6000 branch probability changes

[Jan Hubicka]

> 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

Re: [PATCH] rs6000 branch probability changes

[Jan Hubicka]

> 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