diff mbox series

Fixes for profile count/probability maintenance

Message ID DS7PR21MB3479B004AEFB75D629A471F591FAA@DS7PR21MB3479.namprd21.prod.outlook.com
State New
Headers show
Series Fixes for profile count/probability maintenance | expand

Commit Message

Eugene Rozenfeld Sept. 19, 2023, 6:16 p.m. UTC 
Verifier checks have recently been strengthened to check that
all counts and probabilities are initialized. The checks fired
during autoprofiledbootstrap build and this patch fixes it.

gcc/ChangeLog:

        * auto-profile.cc (afdo_calculate_branch_prob): Fix count comparisons
        * ipa-utils.cc (ipa_merge_profiles): Guard against zero count when
        computing probabilities
        * tree-vect-loop-manip.cc (vect_do_peeling): Guard against zero count
        when scaling loop profile

Tested on x86_64-pc-linux-gnu.

---
 gcc/auto-profile.cc         |  4 ++--
 gcc/ipa-utils.cc            | 16 +++++++++-------
 gcc/tree-vect-loop-manip.cc |  2 +-
 3 files changed, 12 insertions(+), 10 deletions(-)

--
2.25.1

Comments

Richard Biener Sept. 27, 2023, 11:32 a.m. UTC | #1 
On Tue, Sep 19, 2023 at 8:17 PM Eugene Rozenfeld
<Eugene.Rozenfeld@microsoft.com> wrote:
>
> Verifier checks have recently been strengthened to check that
> all counts and probabilities are initialized. The checks fired
> during autoprofiledbootstrap build and this patch fixes it.

OK if Honza doesn't have any comments this week.

Richard.

> gcc/ChangeLog:
>
>         * auto-profile.cc (afdo_calculate_branch_prob): Fix count comparisons
>         * ipa-utils.cc (ipa_merge_profiles): Guard against zero count when
>         computing probabilities
>         * tree-vect-loop-manip.cc (vect_do_peeling): Guard against zero count
>         when scaling loop profile
>
> Tested on x86_64-pc-linux-gnu.
>
> ---
>  gcc/auto-profile.cc         |  4 ++--
>  gcc/ipa-utils.cc            | 16 +++++++++-------
>  gcc/tree-vect-loop-manip.cc |  2 +-
>  3 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
> index ff3b763945c..3e61f36c29b 100644
> --- a/gcc/auto-profile.cc
> +++ b/gcc/auto-profile.cc
> @@ -1434,7 +1434,7 @@ afdo_calculate_branch_prob (bb_set *annotated_bb)
>        else
>          total_count += AFDO_EINFO (e)->get_count ();
>      }
> -    if (num_unknown_succ == 0 && total_count > profile_count::zero ())
> +    if (num_unknown_succ == 0 && total_count > profile_count::zero ().afdo ())
>        {
>         FOR_EACH_EDGE (e, ei, bb->succs)
>           e->probability
> @@ -1571,7 +1571,7 @@ afdo_annotate_cfg (const stmt_set &promoted_stmts)
>        DECL_SOURCE_LOCATION (current_function_decl));
>    afdo_source_profile->mark_annotated (cfun->function_start_locus);
>    afdo_source_profile->mark_annotated (cfun->function_end_locus);
> -  if (max_count > profile_count::zero ())
> +  if (max_count > profile_count::zero ().afdo ())
>      {
>        /* Calculate, propagate count and probability information on CFG.  */
>        afdo_calculate_branch_prob (&annotated_bb);
> diff --git a/gcc/ipa-utils.cc b/gcc/ipa-utils.cc
> index 956c6294fd7..3aaf7e595df 100644
> --- a/gcc/ipa-utils.cc
> +++ b/gcc/ipa-utils.cc
> @@ -651,13 +651,15 @@ ipa_merge_profiles (struct cgraph_node *dst,
>                 {
>                   edge srce = EDGE_SUCC (srcbb, i);
>                   edge dste = EDGE_SUCC (dstbb, i);
> -                 dste->probability =
> -                   dste->probability * dstbb->count.ipa ().probability_in
> -                                                (dstbb->count.ipa ()
> -                                                 + srccount.ipa ())
> -                   + srce->probability * srcbb->count.ipa ().probability_in
> -                                                (dstbb->count.ipa ()
> -                                                 + srccount.ipa ());
> +                 profile_count total = dstbb->count.ipa () + srccount.ipa ();
> +                 if (total.nonzero_p ())
> +                   {
> +                     dste->probability =
> +                       dste->probability * dstbb->count.ipa ().probability_in
> +                                                                       (total)
> +                       + srce->probability * srcbb->count.ipa ().probability_in
> +                                                                       (total);
> +                   }
>                 }
>               dstbb->count = dstbb->count.ipa () + srccount.ipa ();
>             }
> diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
> index 09641901ff1..2608c286e5d 100644
> --- a/gcc/tree-vect-loop-manip.cc
> +++ b/gcc/tree-vect-loop-manip.cc
> @@ -3335,7 +3335,7 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1,
>               free (bbs);
>               free (original_bbs);
>             }
> -         else
> +         else if (old_count.nonzero_p ())
>             scale_loop_profile (epilog, guard_to->count.probability_in (old_count), -1);
>
>           /* Only need to handle basic block before epilog loop if it's not
> --
> 2.25.1
Jan Hubicka Sept. 27, 2023, 11:40 a.m. UTC | #2 
> > gcc/ChangeLog:
> >
> >         * auto-profile.cc (afdo_calculate_branch_prob): Fix count comparisons
> >         * ipa-utils.cc (ipa_merge_profiles): Guard against zero count when
> >         computing probabilities
> >         * tree-vect-loop-manip.cc (vect_do_peeling): Guard against zero count
> >         when scaling loop profile
> >
> > Tested on x86_64-pc-linux-gnu.
> >
> > ---
> >  gcc/auto-profile.cc         |  4 ++--
> >  gcc/ipa-utils.cc            | 16 +++++++++-------
> >  gcc/tree-vect-loop-manip.cc |  2 +-
> >  3 files changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
> > index ff3b763945c..3e61f36c29b 100644
> > --- a/gcc/auto-profile.cc
> > +++ b/gcc/auto-profile.cc
> > @@ -1434,7 +1434,7 @@ afdo_calculate_branch_prob (bb_set *annotated_bb)
> >        else
> >          total_count += AFDO_EINFO (e)->get_count ();
> >      }
> > -    if (num_unknown_succ == 0 && total_count > profile_count::zero ())
> > +    if (num_unknown_succ == 0 && total_count > profile_count::zero ().afdo ())

I think you want nonzero_p() here (it is usual guard to use when
computing probability_in)
> > @@ -651,13 +651,15 @@ ipa_merge_profiles (struct cgraph_node *dst,
> >                 {
> >                   edge srce = EDGE_SUCC (srcbb, i);
> >                   edge dste = EDGE_SUCC (dstbb, i);
> > -                 dste->probability =
> > -                   dste->probability * dstbb->count.ipa ().probability_in
> > -                                                (dstbb->count.ipa ()
> > -                                                 + srccount.ipa ())
> > -                   + srce->probability * srcbb->count.ipa ().probability_in
> > -                                                (dstbb->count.ipa ()
> > -                                                 + srccount.ipa ());
> > +                 profile_count total = dstbb->count.ipa () + srccount.ipa ();
> > +                 if (total.nonzero_p ())
> > +                   {
> > +                     dste->probability =
> > +                       dste->probability * dstbb->count.ipa ().probability_in
> > +                                                                       (total)
> > +                       + srce->probability * srcbb->count.ipa ().probability_in
> > +                                                                       (total);
> > +                   }
> >                 }
> >               dstbb->count = dstbb->count.ipa () + srccount.ipa ();
> >             }
> > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
> > index 09641901ff1..2608c286e5d 100644
> > --- a/gcc/tree-vect-loop-manip.cc
> > +++ b/gcc/tree-vect-loop-manip.cc
> > @@ -3335,7 +3335,7 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1,
> >               free (bbs);
> >               free (original_bbs);
> >             }
> > -         else
> > +         else if (old_count.nonzero_p ())
> >             scale_loop_profile (epilog, guard_to->count.probability_in (old_count), -1);

This is fine.  Sorry for late reaction.  I had some travelling in last
two weeks.
Honza
> >
> >           /* Only need to handle basic block before epilog loop if it's not
> > --
> > 2.25.1
diff mbox series

Patch

diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
index ff3b763945c..3e61f36c29b 100644
--- a/gcc/auto-profile.cc
+++ b/gcc/auto-profile.cc
@@ -1434,7 +1434,7 @@  afdo_calculate_branch_prob (bb_set *annotated_bb)
       else
         total_count += AFDO_EINFO (e)->get_count ();
     }
-    if (num_unknown_succ == 0 && total_count > profile_count::zero ())
+    if (num_unknown_succ == 0 && total_count > profile_count::zero ().afdo ())
       {
        FOR_EACH_EDGE (e, ei, bb->succs)
          e->probability
@@ -1571,7 +1571,7 @@  afdo_annotate_cfg (const stmt_set &promoted_stmts)
       DECL_SOURCE_LOCATION (current_function_decl));
   afdo_source_profile->mark_annotated (cfun->function_start_locus);
   afdo_source_profile->mark_annotated (cfun->function_end_locus);
-  if (max_count > profile_count::zero ())
+  if (max_count > profile_count::zero ().afdo ())
     {
       /* Calculate, propagate count and probability information on CFG.  */
       afdo_calculate_branch_prob (&annotated_bb);
diff --git a/gcc/ipa-utils.cc b/gcc/ipa-utils.cc
index 956c6294fd7..3aaf7e595df 100644
--- a/gcc/ipa-utils.cc
+++ b/gcc/ipa-utils.cc
@@ -651,13 +651,15 @@  ipa_merge_profiles (struct cgraph_node *dst,
                {
                  edge srce = EDGE_SUCC (srcbb, i);
                  edge dste = EDGE_SUCC (dstbb, i);
-                 dste->probability =
-                   dste->probability * dstbb->count.ipa ().probability_in
-                                                (dstbb->count.ipa ()
-                                                 + srccount.ipa ())
-                   + srce->probability * srcbb->count.ipa ().probability_in
-                                                (dstbb->count.ipa ()
-                                                 + srccount.ipa ());
+                 profile_count total = dstbb->count.ipa () + srccount.ipa ();
+                 if (total.nonzero_p ())
+                   {
+                     dste->probability =
+                       dste->probability * dstbb->count.ipa ().probability_in
+                                                                       (total)
+                       + srce->probability * srcbb->count.ipa ().probability_in
+                                                                       (total);
+                   }
                }
              dstbb->count = dstbb->count.ipa () + srccount.ipa ();
            }
diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
index 09641901ff1..2608c286e5d 100644
--- a/gcc/tree-vect-loop-manip.cc
+++ b/gcc/tree-vect-loop-manip.cc
@@ -3335,7 +3335,7 @@  vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1,
              free (bbs);
              free (original_bbs);
            }
-         else
+         else if (old_count.nonzero_p ())
            scale_loop_profile (epilog, guard_to->count.probability_in (old_count), -1);

          /* Only need to handle basic block before epilog loop if it's not