[PATCH AutoFDO/2]Treat ZERO as common profile probability/count

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH AutoFDO/2]Treat ZERO as common profile probability/count

bin.cheng-2
Hi,
In new profile probability/count infra, we have different precision quality categories,
and probabilities/counts of different categories are not supposed to be compared or
calculated.  Though in general is an improvement, it introduces unexpected behavior.
Specifically, class profile_probablity and profile_count themselves are implemented
by comparing probabilities/counts against profile_count::zero().  while zero() is of
profile_precision category, it's always compared different to zero of other precision
categories including afdo.

I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
against probability_count::zero().  2) requires lots of code changes so I went with 1)
in this patch set.  This patch doesn't handle "always" but it might be.

This patch also corrects a minor issue where we try to invert an uninitialized value.

Bootstrap and test on x86_64 in patch set.  Is it OK?

Thanks,
bin

2018-10-31  Bin Cheng  <[hidden email]>

        * expmed.c (emit_store_flag_force): Use profile_probability::always.
        * profile-count.h (profile_probability::always): Add parameter.
        (profile_probability::operator==, profile_count::operator==): Treat
        ZERO as common probability/count regardless of its quality.
        (profile_probability::invert): Don't invert uninitialized probability.

=?UTF-8?B?MDAwMi1IYW5kbGUtWkVSTy1wcm9maWxlLWNvdW50LXByb2ItYXMtYS1nZW5lcmFsLXZhbHVlLWZvLnBhdGNo?= (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Richard Biener-2
On Wed, Oct 31, 2018 at 7:30 AM bin.cheng <[hidden email]> wrote:

>
> Hi,
> In new profile probability/count infra, we have different precision quality categories,
> and probabilities/counts of different categories are not supposed to be compared or
> calculated.  Though in general is an improvement, it introduces unexpected behavior.
> Specifically, class profile_probablity and profile_count themselves are implemented
> by comparing probabilities/counts against profile_count::zero().  while zero() is of
> profile_precision category, it's always compared different to zero of other precision
> categories including afdo.
>
> I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
> of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
> against probability_count::zero().  2) requires lots of code changes so I went with 1)
> in this patch set.  This patch doesn't handle "always" but it might be.
>
> This patch also corrects a minor issue where we try to invert an uninitialized value.
>
> Bootstrap and test on x86_64 in patch set.  Is it OK?

I'll defer on the emit_store_flag_force change, likewise for the zero
handling in
compares - I don't think zeros of different qualities should compare equal.
Would compares against ::always() not have the very same issue?
Likewise ::even(),
::likely(), etc.?  Those always get guessed quality.

The invert change looks OK to me.  The related change to the always() API would
suggest to replace guessed_always() with always (guessed) and also do similar
changes throughout the whole API...

Honza?

Thanks,
Richard.


> Thanks,
> bin
>
> 2018-10-31  Bin Cheng  <[hidden email]>
>
>         * expmed.c (emit_store_flag_force): Use profile_probability::always.
>         * profile-count.h (profile_probability::always): Add parameter.
>         (profile_probability::operator==, profile_count::operator==): Treat
>         ZERO as common probability/count regardless of its quality.
>         (profile_probability::invert): Don't invert uninitialized probability.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Bin.Cheng
On Wed, Oct 31, 2018 at 5:11 PM Richard Biener
<[hidden email]> wrote:

>
> On Wed, Oct 31, 2018 at 7:30 AM bin.cheng <[hidden email]> wrote:
> >
> > Hi,
> > In new profile probability/count infra, we have different precision quality categories,
> > and probabilities/counts of different categories are not supposed to be compared or
> > calculated.  Though in general is an improvement, it introduces unexpected behavior.
> > Specifically, class profile_probablity and profile_count themselves are implemented
> > by comparing probabilities/counts against profile_count::zero().  while zero() is of
> > profile_precision category, it's always compared different to zero of other precision
> > categories including afdo.
> >
> > I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
> > of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
> > against probability_count::zero().  2) requires lots of code changes so I went with 1)
> > in this patch set.  This patch doesn't handle "always" but it might be.
> >
> > This patch also corrects a minor issue where we try to invert an uninitialized value.
> >
> > Bootstrap and test on x86_64 in patch set.  Is it OK?
>
> I'll defer on the emit_store_flag_force change, likewise for the zero
> handling in
> compares - I don't think zeros of different qualities should compare equal.
> Would compares against ::always() not have the very same issue?
> Likewise ::even(),
> ::likely(), etc.?  Those always get guessed quality.
Yes, these values also affected if compared with precise category, but
zero is the major issue.  So 2) makes more sense when checking if a
profile count is_zero/is_likely/is_always etc. regardless of its categories.
Once with Honza's input, I can do some experiments.

Thanks,
bin

>
> The invert change looks OK to me.  The related change to the always() API would
> suggest to replace guessed_always() with always (guessed) and also do similar
> changes throughout the whole API...
>
> Honza?
>
> Thanks,
> Richard.
>
>
> > Thanks,
> > bin
> >
> > 2018-10-31  Bin Cheng  <[hidden email]>
> >
> >         * expmed.c (emit_store_flag_force): Use profile_probability::always.
> >         * profile-count.h (profile_probability::always): Add parameter.
> >         (profile_probability::operator==, profile_count::operator==): Treat
> >         ZERO as common probability/count regardless of its quality.
> >         (profile_probability::invert): Don't invert uninitialized probability.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Jeff Law
In reply to this post by bin.cheng-2
On 10/31/18 12:30 AM, bin.cheng wrote:

> Hi,
> In new profile probability/count infra, we have different precision quality categories,
> and probabilities/counts of different categories are not supposed to be compared or
> calculated.  Though in general is an improvement, it introduces unexpected behavior.
> Specifically, class profile_probablity and profile_count themselves are implemented
> by comparing probabilities/counts against profile_count::zero().  while zero() is of
> profile_precision category, it's always compared different to zero of other precision
> categories including afdo.
>
> I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
> of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
> against probability_count::zero().  2) requires lots of code changes so I went with 1)
> in this patch set.  This patch doesn't handle "always" but it might be.
>
> This patch also corrects a minor issue where we try to invert an uninitialized value.
>
> Bootstrap and test on x86_64 in patch set.  Is it OK?
>
> Thanks,
> bin
>
> 2018-10-31  Bin Cheng  <[hidden email]>
>
> * expmed.c (emit_store_flag_force): Use profile_probability::always.
> * profile-count.h (profile_probability::always): Add parameter.
> (profile_probability::operator==, profile_count::operator==): Treat
> ZERO as common probability/count regardless of its quality.
> (profile_probability::invert): Don't invert uninitialized probability.
>

I'm really not sure the emit_store_flag_force change is right --
essentially without external information I can't see how we can pass in
any probability here other than "we don't know" which is
profile_probability::uninitialized IIUC.

You could potentially make an argument that a 50-50 probability is
reasonable here.  That's profile_probability::even.  But I just don't
see how profile_probability::always is right here.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Bin.Cheng
On Wed, Oct 31, 2018 at 10:36 PM Jeff Law <[hidden email]> wrote:

>
> On 10/31/18 12:30 AM, bin.cheng wrote:
> > Hi,
> > In new profile probability/count infra, we have different precision quality categories,
> > and probabilities/counts of different categories are not supposed to be compared or
> > calculated.  Though in general is an improvement, it introduces unexpected behavior.
> > Specifically, class profile_probablity and profile_count themselves are implemented
> > by comparing probabilities/counts against profile_count::zero().  while zero() is of
> > profile_precision category, it's always compared different to zero of other precision
> > categories including afdo.
> >
> > I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
> > of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
> > against probability_count::zero().  2) requires lots of code changes so I went with 1)
> > in this patch set.  This patch doesn't handle "always" but it might be.
> >
> > This patch also corrects a minor issue where we try to invert an uninitialized value.
> >
> > Bootstrap and test on x86_64 in patch set.  Is it OK?
> >
> > Thanks,
> > bin
> >
> > 2018-10-31  Bin Cheng  <[hidden email]>
> >
> >       * expmed.c (emit_store_flag_force): Use profile_probability::always.
> >       * profile-count.h (profile_probability::always): Add parameter.
> >       (profile_probability::operator==, profile_count::operator==): Treat
> >       ZERO as common probability/count regardless of its quality.
> >       (profile_probability::invert): Don't invert uninitialized probability.
> >
>
> I'm really not sure the emit_store_flag_force change is right --
> essentially without external information I can't see how we can pass in
> any probability here other than "we don't know" which is
> profile_probability::uninitialized IIUC.
>
> You could potentially make an argument that a 50-50 probability is
> reasonable here.  That's profile_probability::even.  But I just don't
> see how profile_probability::always is right here.
Thanks, I realized I mis-spotted that piece of code.  Will discard the part.

Thanks,
bin
>
> jeff
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

bin.cheng-2
In reply to this post by Richard Biener-2
------------------------------------------------------------------
Sender:Richard Biener <[hidden email]>
Sent at:2018 Oct 31 (Wed) 17:11
To:bin.cheng <[hidden email]>; Jan Hubicka <[hidden email]>
Cc:GCC Patches <[hidden email]>
Subject:Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

>
>
> On Wed, Oct 31, 2018 at 7:30 AM bin.cheng <[hidden email]> wrote:
>>
>> Hi,
>> In new profile probability/count infra, we have different precision quality categories,
>> and probabilities/counts of different categories are not supposed to be compared or
>> calculated.  Though in general is an improvement, it introduces unexpected behavior.
>> Specifically, class profile_probablity and profile_count themselves are implemented
>> by comparing probabilities/counts against profile_count::zero().  while zero() is of
>> profile_precision category, it's always compared different to zero of other precision
>> categories including afdo.
>>
>> I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
>> of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
>> against probability_count::zero().  2) requires lots of code changes so I went with 1)
>> in this patch set.  This patch doesn't handle "always" but it might be.
>>
>> This patch also corrects a minor issue where we try to invert an uninitialized value.
>>
>> Bootstrap and test on x86_64 in patch set.  Is it OK?
>
Thanks for all the reviews.

> I'll defer on the emit_store_flag_force change, likewise for the zero

As Jeff pointed out too, this is discarded now.

> handling in
> compares - I don't think zeros of different qualities should compare equal.
> Would compares against ::always() not have the very same issue?
> Likewise ::even(),
> ::likely(), etc.?  Those always get guessed quality.

In this version, I went with the second method which adds never_p/zero_p
member function for probability and count.  Check on ZERO value won't fail
unexpected now.  The patch also makes some other changes that avoids
short-circuit returning on ZERO probability/count and let.

>
> The invert change looks OK to me.  The related change to the always() API would
> suggest to replace guessed_always() with always (guessed) and also do similar
> changes throughout the whole API...
The invert part is split as a standalone patch.  I think adding a default precision
parameter to member functions is a little bit better than having various version
functions here, like you mentioned for from_gcov_type.

Bootstrap and test on x86_64 in patch set.  It fixes all (13) ICE autofdo tests.

Thanks,
bin
2018-11-02  Bin Cheng  <[hidden email]>

        * profile-count.h (profile_probability::always): Add parameter.
        (profile_probability::invert): Don't invert uninitialized probability.

2018-11-02  Bin Cheng  <[hidden email]>

        * profile-count.h (profile_probability::never_p): New.
        (profile_probability::operator+, +=, -, -=, *, *=, /, /=): Check ZERO
        probability using never_p.
        (profile_probability::apply_scale): Likewise.
        (profile_probability::apply): Check using uninitialized_p.
        (profile_count::zero_p): New.
        (profile_count::compatible_p): Check ZERO count using zero_p.
        (profile_count::operator+, +=, <, >, <=, >=, apply_scale): Likewise.
        (profile_count::apply_probability, probability_in): Likewise.
        (profile_count::operator-, -=): Likewise.  Don't quick return if RHS
        profile_count is ZERO.
        (profile_count::max): Don't quick return in case of ZERO count.
        * profile-count.c (profile_count::to_frequency): Check ZERO profile
        probability or count using never_p or zero_p.
        (profile_count::to_cgraph_frequency, to_sreal_scale): Likewise.
        (profile_count::adjust_for_ipa_scaling): Likewise.
        (profile_count::combine_with_ipa_count): Likewise.
        (profile_probability::combine_with_count): Likewise.
        * bb-reorder.c (sanitize_hot_paths): Likewise.
        * cfg.c (update_bb_profile_for_threading): Likewise.
        (scale_bbs_frequencies_profile_count): Likewise.
        * ipa-profile.c (ipa_propagate_frequency_1): Likewise.
        (ipa_propagate_frequency): Likewise.
        * ipa-utility.c (ipa_merge_profiles): Likewise.
        * predict.c (maybe_hot_count_p, probably_never_executed): Likewise.
        (probably_never_executed_bb_p, unlikely_executed_stmt_p): Likewise.
        (combine_predictions_for_bb): Likewise.
        (drop_profile, handle_missing_profiles): Likewise.
        (propagate_unlikely_bbs_forward): Likewise.
        (determine_unlikely_bbs): Likewise.
        (estimate_bb_frequencies): Likewise.
        (compute_function_frequency, force_edge_cold): Likewise.
        * profile.c (compute_branch_probabilities): Likewise.
        * shrink-wrap.c (try_shrink_wrapping): Likewise.
        * tree-ssa-loop-manip.c (scale_dominated_blocks_in_loop): Likewise.
        (tree_transform_and_unroll_loop): Likewise.
        * tree-ssa-threadupdate.c (update_profile): Likewise.
        * tree-vect-loop.c (scale_profile_for_vect_loop): Likewise.

=?UTF-8?B?MDAwMi1Eb250LWludmVydC11bmluaXRpYWxpemVkLXZhbHVlLnBhdGNo?= (1K) Download Attachment
=?UTF-8?B?MDAwMy1DaGVjay16ZXJvLXByb2JhYmlsaXR5LWNvdW50LXVzaW5nLW1lbWJlci1mdW5jdGlvbi5wYXRjaA==?= (38K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Jan Hubicka-2
diff --git a/gcc/profile-count.h b/gcc/profile-count.h
index f4d0c340a0a..4289bc5a004 100644
--- a/gcc/profile-count.h
+++ b/gcc/profile-count.h
@@ -200,11 +200,11 @@ public:
       ret.m_quality = profile_guessed;
       return ret;
     }
-  static profile_probability always ()
+  static profile_probability always (enum profile_quality q = profile_precise)

There are functions to convert value into given precision. If you wnat
to guess that something is always taken, you write
profile_probability::always().guessed ()
So for autofdo we only need to add .atofdo() conversion method.

@@ -459,10 +459,12 @@ public:
       return RDIV (val * m_val, max_probability);
     }
 
-  /* Return 1-*THIS.  */
+  /* Return 1-*THIS.  It's meaningless to invert an uninitialized value.  */
   profile_probability invert () const
     {
-      return profile_probability::always() - *this;
+      if (! initialized_p ())
+ return *this;
+      return profile_probability::always (m_quality) - *this;

How this changes the behaviour? If THIS is uninitialied
profile_probability::alwyas() will return uninitialized which seem
to make sense.
If you have value of some quality it will merge the qualitis
and will return value in corresponding m_quality.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Jan Hubicka-2
In reply to this post by bin.cheng-2
diff --git a/gcc/profile-count.h b/gcc/profile-count.h
index 4289bc5a004..2b5e3269250 100644
--- a/gcc/profile-count.h
+++ b/gcc/profile-count.h
@@ -218,6 +218,11 @@ public:
     }
 
 
+  /* Return true if value is zero.  */
+  bool never_p () const
+    {
+      return m_val == 0;
+    }
   /* Return true if value has been initialized.  */
   bool initialized_p () const
     {
@@ -288,9 +293,9 @@ public:
     }
   profile_probability operator+ (const profile_probability &other) const
     {
-      if (other == profile_probability::never ())
+      if (other.never_p ())
  return *this;
-      if (*this == profile_probability::never ())
+      if (this->never_p ())

This is not correct change.  If you add guessed 0 to precise 0,
the result needs to be guessed 0 because we are no longer sure the code
will not get executed.  This is why all the checks here go explicitly
to profile_probability::never.

Honza
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Bin.Cheng
On Mon, Nov 5, 2018 at 10:40 PM Jan Hubicka <[hidden email]> wrote:

>
> diff --git a/gcc/profile-count.h b/gcc/profile-count.h
> index 4289bc5a004..2b5e3269250 100644
> --- a/gcc/profile-count.h
> +++ b/gcc/profile-count.h
> @@ -218,6 +218,11 @@ public:
>      }
>
>
> +  /* Return true if value is zero.  */
> +  bool never_p () const
> +    {
> +      return m_val == 0;
> +    }
>    /* Return true if value has been initialized.  */
>    bool initialized_p () const
>      {
> @@ -288,9 +293,9 @@ public:
>      }
>    profile_probability operator+ (const profile_probability &other) const
>      {
> -      if (other == profile_probability::never ())
> +      if (other.never_p ())
>         return *this;
> -      if (*this == profile_probability::never ())
> +      if (this->never_p ())
>
> This is not correct change.  If you add guessed 0 to precise 0,
> the result needs to be guessed 0 because we are no longer sure the code
> will not get executed.  This is why all the checks here go explicitly
> to profile_probability::never.
Hmm, so precise 0 means the code can never get executed? I also noticed
that in predict.c there are lots of direct assignment of profile_count::zero as:
propagation_unlikely_bbs_forward (void)
{
  //...
  bb->count = profile_count::zero ();
  //...
}
This generally promote profile_count::zero from lower precision to precise
precision, but function name/comment seems targeting unlikely executed
code, rather than never executed.  Is this inconsistent?

Thanks,
bin

>
> Honza
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

bin.cheng-2
In reply to this post by Richard Biener-2
Sender:Jan Hubicka <[hidden email]>
Sent at:2018 Nov 5 (Mon) 22:21
To:Richard Biener <[hidden email]>
Cc:bin.cheng <[hidden email]>; GCC Patches <[hidden email]>
Subject:Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count
 

>
> > On Wed, Oct 31, 2018 at 7:30 AM bin.cheng <[hidden email]> wrote:
> > >
> > > Hi,
> > > In new profile probability/count infra, we have different precision quality categories,
> > > and probabilities/counts of different categories are not supposed to be compared or
> > > calculated.  Though in general is an improvement, it introduces unexpected behavior.
> > > Specifically, class profile_probablity and profile_count themselves are implemented
> > > by comparing probabilities/counts against profile_count::zero().  while zero() is of
> > > profile_precision category, it's always compared different to zero of other precision
> > > categories including afdo.
> > >
> > > I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
> > > of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
> > > against probability_count::zero().  2) requires lots of code changes so I went with 1)
> > > in this patch set.  This patch doesn't handle "always" but it might be.
> > >
> > > This patch also corrects a minor issue where we try to invert an uninitialized value.
> > >
> > > Bootstrap and test on x86_64 in patch set.  Is it OK?
> >
> > I'll defer on the emit_store_flag_force change, likewise for the zero
> > handling in
> > compares - I don't think zeros of different qualities should compare equal.
> > Would compares against ::always() not have the very same issue?
> > Likewise ::even(),
> > ::likely(), etc.?  Those always get guessed quality.
> >
> > The invert change looks OK to me.  The related change to the always() API would
> > suggest to replace guessed_always() with always (guessed) and also do similar
> > changes throughout the whole API...
> >
> > Honza?
>
> The zeros are really differenct zeros.  profile_count::zero makes us to
> drop the basic block into cold section because we know that it won't be
> executed in normal run of program (either we have accurate profile
> feedback or by proving that the program is on way to crash or user
> annotated cold section).  Having guessed zero or auto-fdo zero won't
> make us to do such agressive size optimization.
> This is important since those zeros relatively commonly happens by
> accident and thus if we dropped all the code to cold section the cold
> section would be visited relativel often during execution of program
> which would eliminate its need.
>
> Most comparsion in profile-count.h which goes agains profile_count==zero
> are realy intended to pass only for this "aboslute zero". They bypass
> the precision adjusmtents which normally happen when you merge values
> of different precision.
>
> What kind of unexpected behaviour are you seeing?
> We already have nonzero_p which is what we use when we want to know that
> count is non-zero in some sense of precision.
Hi Honza,
Sorry for letting this slip away.  So in case of AutoFDO, due to the nature
of sampling, lots of funcs/bbs are annotated with zero profile_count in afdo
precision, and we have checks against zero profile_count in precise precision
All these checks end up with false and cause issues.  Take the code in
update_profiling_info as an example:

update_profiling_info (struct cgraph_node *orig_node,
                       struct cgraph_node *new_node)
{
   struct cgraph_edge *cs;
   struct caller_statistics stats;
   profile_count new_sum, orig_sum;
   profile_count remainder, orig_node_count = orig_node->count;

   if (!(orig_node_count.ipa () > profile_count::zero ()))
     return;
   //...
   for (cs = new_node->callees; cs; cs = cs->next_callee)
     cs->count = cs->count.apply_scale (new_sum, orig_node_count);

Since we also have below code in profile_count::operator>,
      if (other == profile_count::zero ())
        return !(*this == profile_count::zero ());

If orig_node_count is afdo zero, the above zero check for orig_node_count
returns false, we end up with passing zero density to apply_scale issue and
asserting.

In this updated patch, I restrcited changes only to profile_count::operator
<, >, <= and >=.  Plus, I think there is a latent typo in operator>= because
current code return TRUE if '*this' is precise zero and 'other' is precise
non-zero.
@@ -879,7 +879,7 @@ public:
       if (other == profile_count::zero ())
        return true;
       if (*this == profile_count::zero ())
-       return !(other == profile_count::zero ());
+       return !other.nonzero_p ();

Bootstrap and test on x86_64 along with other patches.

Thanks,
bin

2018-11-19  Bin Cheng  <[hidden email]>

        * profile-count.h (profile_count::operator<, >, <=): Check ZERO count
        using nonzero_p.
        (profile_count::oeprator>=): Invert return condition when *this is
        precise zero.  Check ZERO count in that condition using nonzero_p.

=?UTF-8?B?MDAwMy1DaGVjay1aRVJPLXByb2ZpbGUtY291bnQtcmVnYXJkbGVzcy1vZi1wcmVjaXNpb24ucGF0Y2g=?= (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Jan Hubicka-2
> On Tue, Nov 20, 2018 at 6:55 PM bin.cheng <[hidden email]> wrote:
> >
> > Sender:Jan Hubicka <[hidden email]>
> > Sent at:2018 Nov 5 (Mon) 22:21
> > To:Richard Biener <[hidden email]>
> > Cc:bin.cheng <[hidden email]>; GCC Patches <[hidden email]>
> > Subject:Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count
> >
> > >
> > > > On Wed, Oct 31, 2018 at 7:30 AM bin.cheng <[hidden email]> wrote:
> > > > >
> > > > > Hi,
> > > > > In new profile probability/count infra, we have different precision quality categories,
> > > > > and probabilities/counts of different categories are not supposed to be compared or
> > > > > calculated.  Though in general is an improvement, it introduces unexpected behavior.
> > > > > Specifically, class profile_probablity and profile_count themselves are implemented
> > > > > by comparing probabilities/counts against profile_count::zero().  while zero() is of
> > > > > profile_precision category, it's always compared different to zero of other precision
> > > > > categories including afdo.
> > > > >
> > > > > I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
> > > > > of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
> > > > > against probability_count::zero().  2) requires lots of code changes so I went with 1)
> > > > > in this patch set.  This patch doesn't handle "always" but it might be.
> > > > >
> > > > > This patch also corrects a minor issue where we try to invert an uninitialized value.
> > > > >
> > > > > Bootstrap and test on x86_64 in patch set.  Is it OK?
> > > >
> > > > I'll defer on the emit_store_flag_force change, likewise for the zero
> > > > handling in
> > > > compares - I don't think zeros of different qualities should compare equal.
> > > > Would compares against ::always() not have the very same issue?
> > > > Likewise ::even(),
> > > > ::likely(), etc.?  Those always get guessed quality.
> > > >
> > > > The invert change looks OK to me.  The related change to the always() API would
> > > > suggest to replace guessed_always() with always (guessed) and also do similar
> > > > changes throughout the whole API...
> > > >
> > > > Honza?
> > >
> > > The zeros are really differenct zeros.  profile_count::zero makes us to
> > > drop the basic block into cold section because we know that it won't be
> > > executed in normal run of program (either we have accurate profile
> > > feedback or by proving that the program is on way to crash or user
> > > annotated cold section).  Having guessed zero or auto-fdo zero won't
> > > make us to do such agressive size optimization.
> > > This is important since those zeros relatively commonly happens by
> > > accident and thus if we dropped all the code to cold section the cold
> > > section would be visited relativel often during execution of program
> > > which would eliminate its need.
> > >
> > > Most comparsion in profile-count.h which goes agains profile_count==zero
> > > are realy intended to pass only for this "aboslute zero". They bypass
> > > the precision adjusmtents which normally happen when you merge values
> > > of different precision.
> > >
> > > What kind of unexpected behaviour are you seeing?
> > > We already have nonzero_p which is what we use when we want to know that
> > > count is non-zero in some sense of precision.
> > Hi Honza,
> > Sorry for letting this slip away.  So in case of AutoFDO, due to the nature
> > of sampling, lots of funcs/bbs are annotated with zero profile_count in afdo
> > precision, and we have checks against zero profile_count in precise precision
> > All these checks end up with false and cause issues.  Take the code in
> > update_profiling_info as an example:
> >
> > update_profiling_info (struct cgraph_node *orig_node,
> >                        struct cgraph_node *new_node)
> > {
> >    struct cgraph_edge *cs;
> >    struct caller_statistics stats;
> >    profile_count new_sum, orig_sum;
> >    profile_count remainder, orig_node_count = orig_node->count;
> >
> >    if (!(orig_node_count.ipa () > profile_count::zero ()))
> >      return;
> >    //...
> >    for (cs = new_node->callees; cs; cs = cs->next_callee)
> >      cs->count = cs->count.apply_scale (new_sum, orig_node_count);
> >
> > Since we also have below code in profile_count::operator>,
> >       if (other == profile_count::zero ())
> >         return !(*this == profile_count::zero ());
> >
> > If orig_node_count is afdo zero, the above zero check for orig_node_count
> > returns false, we end up with passing zero density to apply_scale issue and
> > asserting.
> >
> > In this updated patch, I restrcited changes only to profile_count::operator
> > <, >, <= and >=.  Plus, I think there is a latent typo in operator>= because
> > current code return TRUE if '*this' is precise zero and 'other' is precise
> > non-zero.
> > @@ -879,7 +879,7 @@ public:
> >        if (other == profile_count::zero ())
> >         return true;
> >        if (*this == profile_count::zero ())
> > -       return !(other == profile_count::zero ());
> > +       return !other.nonzero_p ();

We already have

True:
 profile_count::zero < any other value
 any other value > profile_count::zero
 profile_count::zero <= any initialized value
 profile_count::zero <= profile_count::zero
 any initialized value >= profile_count::zero

false
 profile_count::zero > any other value
 any other value < profile_count::zero

You are right about typo in >=, it should be:

Index: profile-count.h
===================================================================
--- profile-count.h     (revision 266450)
+++ profile-count.h     (working copy)
@@ -879,7 +879,7 @@
       if (other == profile_count::zero ())
        return true;
       if (*this == profile_count::zero ())
-       return !(other == profile_count::zero ());
+       return other == profile_count::zero ();
       gcc_checking_assert (compatible_p (other));
       return m_val >= other.m_val;
     }

With your patch we get false for:
  profile_count::zero < guessed/auto_fdo/other 0
  guessed/auto_fdo/other > profile_count::zero
  guessed/auto_fdo/other <= profile_count::zero
  profile_count::zero >= profile_count::zero

The original idea was to intentionally make profile_count::zero smaller
than any toher types of initialized values, since it is more strict hint
that the path will not be taken.
For example in bb_reorder if you end up with "funny" profile with two
exit edges one having profile_count::zero and other being zero as result
of (unsucesfull) profile updates it is still better idea to pick the
profile_count::zero for taken edge.  With your patch it will end up
picking either of the paths.

How the patch helps to your situation?

The fix for >= is OK, thanks for spotting that!
Honza

> >
> > Bootstrap and test on x86_64 along with other patches.
> Ping.
>
> Thanks,
> bin
> >
> > Thanks,
> > bin
> >
> > 2018-11-19  Bin Cheng  <[hidden email]>
> >
> >         * profile-count.h (profile_count::operator<, >, <=): Check ZERO count
> >         using nonzero_p.
> >         (profile_count::oeprator>=): Invert return condition when *this is
> >         precise zero.  Check ZERO count in that condition using nonzero_p.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Bin.Cheng
On Thu, Nov 29, 2018 at 12:20 AM Jan Hubicka <[hidden email]> wrote:

>
> > On Tue, Nov 20, 2018 at 6:55 PM bin.cheng <[hidden email]> wrote:
> > >
> > > Sender:Jan Hubicka <[hidden email]>
> > > Sent at:2018 Nov 5 (Mon) 22:21
> > > To:Richard Biener <[hidden email]>
> > > Cc:bin.cheng <[hidden email]>; GCC Patches <[hidden email]>
> > > Subject:Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count
> > >
> > > >
> > > > > On Wed, Oct 31, 2018 at 7:30 AM bin.cheng <[hidden email]> wrote:
> > > > > >
> > > > > > Hi,
> > > > > > In new profile probability/count infra, we have different precision quality categories,
> > > > > > and probabilities/counts of different categories are not supposed to be compared or
> > > > > > calculated.  Though in general is an improvement, it introduces unexpected behavior.
> > > > > > Specifically, class profile_probablity and profile_count themselves are implemented
> > > > > > by comparing probabilities/counts against profile_count::zero().  while zero() is of
> > > > > > profile_precision category, it's always compared different to zero of other precision
> > > > > > categories including afdo.
> > > > > >
> > > > > > I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
> > > > > > of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
> > > > > > against probability_count::zero().  2) requires lots of code changes so I went with 1)
> > > > > > in this patch set.  This patch doesn't handle "always" but it might be.
> > > > > >
> > > > > > This patch also corrects a minor issue where we try to invert an uninitialized value.
> > > > > >
> > > > > > Bootstrap and test on x86_64 in patch set.  Is it OK?
> > > > >
> > > > > I'll defer on the emit_store_flag_force change, likewise for the zero
> > > > > handling in
> > > > > compares - I don't think zeros of different qualities should compare equal.
> > > > > Would compares against ::always() not have the very same issue?
> > > > > Likewise ::even(),
> > > > > ::likely(), etc.?  Those always get guessed quality.
> > > > >
> > > > > The invert change looks OK to me.  The related change to the always() API would
> > > > > suggest to replace guessed_always() with always (guessed) and also do similar
> > > > > changes throughout the whole API...
> > > > >
> > > > > Honza?
> > > >
> > > > The zeros are really differenct zeros.  profile_count::zero makes us to
> > > > drop the basic block into cold section because we know that it won't be
> > > > executed in normal run of program (either we have accurate profile
> > > > feedback or by proving that the program is on way to crash or user
> > > > annotated cold section).  Having guessed zero or auto-fdo zero won't
> > > > make us to do such agressive size optimization.
> > > > This is important since those zeros relatively commonly happens by
> > > > accident and thus if we dropped all the code to cold section the cold
> > > > section would be visited relativel often during execution of program
> > > > which would eliminate its need.
> > > >
> > > > Most comparsion in profile-count.h which goes agains profile_count==zero
> > > > are realy intended to pass only for this "aboslute zero". They bypass
> > > > the precision adjusmtents which normally happen when you merge values
> > > > of different precision.
> > > >
> > > > What kind of unexpected behaviour are you seeing?
> > > > We already have nonzero_p which is what we use when we want to know that
> > > > count is non-zero in some sense of precision.
> > > Hi Honza,
> > > Sorry for letting this slip away.  So in case of AutoFDO, due to the nature
> > > of sampling, lots of funcs/bbs are annotated with zero profile_count in afdo
> > > precision, and we have checks against zero profile_count in precise precision
> > > All these checks end up with false and cause issues.  Take the code in
> > > update_profiling_info as an example:
> > >
> > > update_profiling_info (struct cgraph_node *orig_node,
> > >                        struct cgraph_node *new_node)
> > > {
> > >    struct cgraph_edge *cs;
> > >    struct caller_statistics stats;
> > >    profile_count new_sum, orig_sum;
> > >    profile_count remainder, orig_node_count = orig_node->count;
> > >
> > >    if (!(orig_node_count.ipa () > profile_count::zero ()))
> > >      return;
> > >    //...
> > >    for (cs = new_node->callees; cs; cs = cs->next_callee)
> > >      cs->count = cs->count.apply_scale (new_sum, orig_node_count);
> > >
> > > Since we also have below code in profile_count::operator>,
> > >       if (other == profile_count::zero ())
> > >         return !(*this == profile_count::zero ());
> > >
> > > If orig_node_count is afdo zero, the above zero check for orig_node_count
> > > returns false, we end up with passing zero density to apply_scale issue and
> > > asserting.
> > >
> > > In this updated patch, I restrcited changes only to profile_count::operator
> > > <, >, <= and >=.  Plus, I think there is a latent typo in operator>= because
> > > current code return TRUE if '*this' is precise zero and 'other' is precise
> > > non-zero.
> > > @@ -879,7 +879,7 @@ public:
> > >        if (other == profile_count::zero ())
> > >         return true;
> > >        if (*this == profile_count::zero ())
> > > -       return !(other == profile_count::zero ());
> > > +       return !other.nonzero_p ();
>
> We already have
>
> True:
>  profile_count::zero < any other value
>  any other value > profile_count::zero
>  profile_count::zero <= any initialized value
>  profile_count::zero <= profile_count::zero
>  any initialized value >= profile_count::zero
>
> false
>  profile_count::zero > any other value
>  any other value < profile_count::zero
>
> You are right about typo in >=, it should be:
>
> Index: profile-count.h
> ===================================================================
> --- profile-count.h     (revision 266450)
> +++ profile-count.h     (working copy)
> @@ -879,7 +879,7 @@
>        if (other == profile_count::zero ())
>         return true;
>        if (*this == profile_count::zero ())
> -       return !(other == profile_count::zero ());
> +       return other == profile_count::zero ();
>        gcc_checking_assert (compatible_p (other));
>        return m_val >= other.m_val;
>      }
>
> With your patch we get false for:
>   profile_count::zero < guessed/auto_fdo/other 0
>   guessed/auto_fdo/other > profile_count::zero
>   guessed/auto_fdo/other <= profile_count::zero
>   profile_count::zero >= profile_count::zero
>
> The original idea was to intentionally make profile_count::zero smaller
> than any toher types of initialized values, since it is more strict hint
> that the path will not be taken.
> For example in bb_reorder if you end up with "funny" profile with two
> exit edges one having profile_count::zero and other being zero as result
> of (unsucesfull) profile updates it is still better idea to pick the
> profile_count::zero for taken edge.  With your patch it will end up
> picking either of the paths.
>
> How the patch helps to your situation?
Hi Honza, thanks very much for elaborating.  Issue in case of autofdo
is as described in last message:
Given update_profiling_info implemented as below:

update_profiling_info (struct cgraph_node *orig_node,
                       struct cgraph_node *new_node)
{
   struct cgraph_edge *cs;
   struct caller_statistics stats;
   profile_count new_sum, orig_sum;
   profile_count remainder, orig_node_count = orig_node->count;

   //*****Operator ">" returns true if orig_node_count == autofdo.zero.
   if (!(orig_node_count.ipa () > profile_count::zero ()))
     return;
   //...
   for (cs = new_node->callees; cs; cs = cs->next_callee)
     //*****Result in apply_scale called with autofdo.zero as the 2nd argument.
     cs->count = cs->count.apply_scale (new_sum, orig_node_count);

Also apply_scale is implemented as:
  profile_count apply_scale (profile_count num, profile_count den) const
    {
      if (*this == profile_count::zero ())
        return *this;
      if (num == profile_count::zero ())
        return num;
      if (!initialized_p () || !num.initialized_p () || !den.initialized_p ())
        return profile_count::uninitialized ();
      if (num == den)
        return *this;
      gcc_checking_assert (den.m_val);

Here we have (num != zero && den == autofdo.zero), it triggers the
gcc_checking_assert.
According to your explanation, guess we need to call force_nonzero for
orig_node_count before calling apply_scale, right?

Thanks,
bin
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Bin.Cheng
On Tue, Dec 4, 2018 at 4:40 PM Bin.Cheng <[hidden email]> wrote:

>
> On Thu, Nov 29, 2018 at 12:20 AM Jan Hubicka <[hidden email]> wrote:
> >
> > > On Tue, Nov 20, 2018 at 6:55 PM bin.cheng <[hidden email]> wrote:
> > > >
> > > > Sender:Jan Hubicka <[hidden email]>
> > > > Sent at:2018 Nov 5 (Mon) 22:21
> > > > To:Richard Biener <[hidden email]>
> > > > Cc:bin.cheng <[hidden email]>; GCC Patches <[hidden email]>
> > > > Subject:Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count
> > > >
> > > > >
> > > > > > On Wed, Oct 31, 2018 at 7:30 AM bin.cheng <[hidden email]> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > > In new profile probability/count infra, we have different precision quality categories,
> > > > > > > and probabilities/counts of different categories are not supposed to be compared or
> > > > > > > calculated.  Though in general is an improvement, it introduces unexpected behavior.
> > > > > > > Specifically, class profile_probablity and profile_count themselves are implemented
> > > > > > > by comparing probabilities/counts against profile_count::zero().  while zero() is of
> > > > > > > profile_precision category, it's always compared different to zero of other precision
> > > > > > > categories including afdo.
> > > > > > >
> > > > > > > I can see two ways fixing this: 1) Treat zero as a common probability/count regardless
> > > > > > > of its category; 2) Provide an "is_zero" method rather than relying on "==" comparison
> > > > > > > against probability_count::zero().  2) requires lots of code changes so I went with 1)
> > > > > > > in this patch set.  This patch doesn't handle "always" but it might be.
> > > > > > >
> > > > > > > This patch also corrects a minor issue where we try to invert an uninitialized value.
> > > > > > >
> > > > > > > Bootstrap and test on x86_64 in patch set.  Is it OK?
> > > > > >
> > > > > > I'll defer on the emit_store_flag_force change, likewise for the zero
> > > > > > handling in
> > > > > > compares - I don't think zeros of different qualities should compare equal.
> > > > > > Would compares against ::always() not have the very same issue?
> > > > > > Likewise ::even(),
> > > > > > ::likely(), etc.?  Those always get guessed quality.
> > > > > >
> > > > > > The invert change looks OK to me.  The related change to the always() API would
> > > > > > suggest to replace guessed_always() with always (guessed) and also do similar
> > > > > > changes throughout the whole API...
> > > > > >
> > > > > > Honza?
> > > > >
> > > > > The zeros are really differenct zeros.  profile_count::zero makes us to
> > > > > drop the basic block into cold section because we know that it won't be
> > > > > executed in normal run of program (either we have accurate profile
> > > > > feedback or by proving that the program is on way to crash or user
> > > > > annotated cold section).  Having guessed zero or auto-fdo zero won't
> > > > > make us to do such agressive size optimization.
> > > > > This is important since those zeros relatively commonly happens by
> > > > > accident and thus if we dropped all the code to cold section the cold
> > > > > section would be visited relativel often during execution of program
> > > > > which would eliminate its need.
> > > > >
> > > > > Most comparsion in profile-count.h which goes agains profile_count==zero
> > > > > are realy intended to pass only for this "aboslute zero". They bypass
> > > > > the precision adjusmtents which normally happen when you merge values
> > > > > of different precision.
> > > > >
> > > > > What kind of unexpected behaviour are you seeing?
> > > > > We already have nonzero_p which is what we use when we want to know that
> > > > > count is non-zero in some sense of precision.
> > > > Hi Honza,
> > > > Sorry for letting this slip away.  So in case of AutoFDO, due to the nature
> > > > of sampling, lots of funcs/bbs are annotated with zero profile_count in afdo
> > > > precision, and we have checks against zero profile_count in precise precision
> > > > All these checks end up with false and cause issues.  Take the code in
> > > > update_profiling_info as an example:
> > > >
> > > > update_profiling_info (struct cgraph_node *orig_node,
> > > >                        struct cgraph_node *new_node)
> > > > {
> > > >    struct cgraph_edge *cs;
> > > >    struct caller_statistics stats;
> > > >    profile_count new_sum, orig_sum;
> > > >    profile_count remainder, orig_node_count = orig_node->count;
> > > >
> > > >    if (!(orig_node_count.ipa () > profile_count::zero ()))
> > > >      return;
> > > >    //...
> > > >    for (cs = new_node->callees; cs; cs = cs->next_callee)
> > > >      cs->count = cs->count.apply_scale (new_sum, orig_node_count);
> > > >
> > > > Since we also have below code in profile_count::operator>,
> > > >       if (other == profile_count::zero ())
> > > >         return !(*this == profile_count::zero ());
> > > >
> > > > If orig_node_count is afdo zero, the above zero check for orig_node_count
> > > > returns false, we end up with passing zero density to apply_scale issue and
> > > > asserting.
> > > >
> > > > In this updated patch, I restrcited changes only to profile_count::operator
> > > > <, >, <= and >=.  Plus, I think there is a latent typo in operator>= because
> > > > current code return TRUE if '*this' is precise zero and 'other' is precise
> > > > non-zero.
> > > > @@ -879,7 +879,7 @@ public:
> > > >        if (other == profile_count::zero ())
> > > >         return true;
> > > >        if (*this == profile_count::zero ())
> > > > -       return !(other == profile_count::zero ());
> > > > +       return !other.nonzero_p ();
> >
> > We already have
> >
> > True:
> >  profile_count::zero < any other value
> >  any other value > profile_count::zero
> >  profile_count::zero <= any initialized value
> >  profile_count::zero <= profile_count::zero
> >  any initialized value >= profile_count::zero
> >
> > false
> >  profile_count::zero > any other value
> >  any other value < profile_count::zero
> >
> > You are right about typo in >=, it should be:
> >
> > Index: profile-count.h
> > ===================================================================
> > --- profile-count.h     (revision 266450)
> > +++ profile-count.h     (working copy)
> > @@ -879,7 +879,7 @@
> >        if (other == profile_count::zero ())
> >         return true;
> >        if (*this == profile_count::zero ())
> > -       return !(other == profile_count::zero ());
> > +       return other == profile_count::zero ();
> >        gcc_checking_assert (compatible_p (other));
> >        return m_val >= other.m_val;
> >      }
> >
> > With your patch we get false for:
> >   profile_count::zero < guessed/auto_fdo/other 0
> >   guessed/auto_fdo/other > profile_count::zero
> >   guessed/auto_fdo/other <= profile_count::zero
> >   profile_count::zero >= profile_count::zero
> >
> > The original idea was to intentionally make profile_count::zero smaller
> > than any toher types of initialized values, since it is more strict hint
> > that the path will not be taken.
> > For example in bb_reorder if you end up with "funny" profile with two
> > exit edges one having profile_count::zero and other being zero as result
> > of (unsucesfull) profile updates it is still better idea to pick the
> > profile_count::zero for taken edge.  With your patch it will end up
> > picking either of the paths.
> >
> > How the patch helps to your situation?
> Hi Honza, thanks very much for elaborating.  Issue in case of autofdo
> is as described in last message:
> Given update_profiling_info implemented as below:
>
> update_profiling_info (struct cgraph_node *orig_node,
>                        struct cgraph_node *new_node)
> {
>    struct cgraph_edge *cs;
>    struct caller_statistics stats;
>    profile_count new_sum, orig_sum;
>    profile_count remainder, orig_node_count = orig_node->count;
>
>    //*****Operator ">" returns true if orig_node_count == autofdo.zero.
>    if (!(orig_node_count.ipa () > profile_count::zero ()))
>      return;
>    //...
>    for (cs = new_node->callees; cs; cs = cs->next_callee)
>      //*****Result in apply_scale called with autofdo.zero as the 2nd argument.
>      cs->count = cs->count.apply_scale (new_sum, orig_node_count);
>
> Also apply_scale is implemented as:
>   profile_count apply_scale (profile_count num, profile_count den) const
>     {
>       if (*this == profile_count::zero ())
>         return *this;
>       if (num == profile_count::zero ())
>         return num;
>       if (!initialized_p () || !num.initialized_p () || !den.initialized_p ())
>         return profile_count::uninitialized ();
>       if (num == den)
>         return *this;
>       gcc_checking_assert (den.m_val);
>
> Here we have (num != zero && den == autofdo.zero), it triggers the
> gcc_checking_assert.
> According to your explanation, guess we need to call force_nonzero for
> orig_node_count before calling apply_scale, right?

Hi Honza,
I have committed the typo fix as revision 266885.
Also I followed your suggestion (IIUC) by calling
profile_count::adjust_for_ipa_scaling for zero den in function
update_profiling_info.  It works and does make more sense than
changing the global zero check logic.
Patch tested as before, is it ok?

Thanks,
bin

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 4471bae11c7..5074ef63da1 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -3715,9 +3715,11 @@ update_profiling_info (struct cgraph_node *orig_node,
   new_sum = orig_node_count.combine_with_ipa_count (new_sum);
   orig_node->count = remainder;

+  profile_count::adjust_for_ipa_scaling (&new_sum, &orig_node_count);
   for (cs = new_node->callees; cs; cs = cs->next_callee)
     cs->count = cs->count.apply_scale (new_sum, orig_node_count);

+  profile_count::adjust_for_ipa_scaling (&remainder, &orig_node_count);
   for (cs = orig_node->callees; cs; cs = cs->next_callee)
     cs->count = cs->count.apply_scale (remainder, orig_node_count);

2018-12-07  Bin Cheng  <[hidden email]>

        * ipa-cp.c (update_profiling_info): Call adjust_for_ipa_scaling for
        zero profile count.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Jan Hubicka-2
> Hi Honza,
> I have committed the typo fix as revision 266885.
> Also I followed your suggestion (IIUC) by calling
> profile_count::adjust_for_ipa_scaling for zero den in function
> update_profiling_info.  It works and does make more sense than
> changing the global zero check logic.
> Patch tested as before, is it ok?

Thanks, patch is OK.
What is situation with AutoFDO now? It would be very nice to get it
fixed for the release :)

Honza

>
> Thanks,
> bin
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 4471bae11c7..5074ef63da1 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -3715,9 +3715,11 @@ update_profiling_info (struct cgraph_node *orig_node,
>    new_sum = orig_node_count.combine_with_ipa_count (new_sum);
>    orig_node->count = remainder;
>
> +  profile_count::adjust_for_ipa_scaling (&new_sum, &orig_node_count);
>    for (cs = new_node->callees; cs; cs = cs->next_callee)
>      cs->count = cs->count.apply_scale (new_sum, orig_node_count);
>
> +  profile_count::adjust_for_ipa_scaling (&remainder, &orig_node_count);
>    for (cs = orig_node->callees; cs; cs = cs->next_callee)
>      cs->count = cs->count.apply_scale (remainder, orig_node_count);
>
> 2018-12-07  Bin Cheng  <[hidden email]>
>
>         * ipa-cp.c (update_profiling_info): Call adjust_for_ipa_scaling for
>         zero profile count.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH AutoFDO/2]Treat ZERO as common profile probability/count

Bin.Cheng
On Sat, Dec 8, 2018 at 12:57 AM Jan Hubicka <[hidden email]> wrote:

>
> > Hi Honza,
> > I have committed the typo fix as revision 266885.
> > Also I followed your suggestion (IIUC) by calling
> > profile_count::adjust_for_ipa_scaling for zero den in function
> > update_profiling_info.  It works and does make more sense than
> > changing the global zero check logic.
> > Patch tested as before, is it ok?
>
> Thanks, patch is OK.
> What is situation with AutoFDO now? It would be very nice to get it
> fixed for the release :)
Jeff already approved the major patch, I just need to do minor
updates.  I will post two more small patches soon.

Thanks,
bin

>
> Honza
> >
> > Thanks,
> > bin
> >
> > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> > index 4471bae11c7..5074ef63da1 100644
> > --- a/gcc/ipa-cp.c
> > +++ b/gcc/ipa-cp.c
> > @@ -3715,9 +3715,11 @@ update_profiling_info (struct cgraph_node *orig_node,
> >    new_sum = orig_node_count.combine_with_ipa_count (new_sum);
> >    orig_node->count = remainder;
> >
> > +  profile_count::adjust_for_ipa_scaling (&new_sum, &orig_node_count);
> >    for (cs = new_node->callees; cs; cs = cs->next_callee)
> >      cs->count = cs->count.apply_scale (new_sum, orig_node_count);
> >
> > +  profile_count::adjust_for_ipa_scaling (&remainder, &orig_node_count);
> >    for (cs = orig_node->callees; cs; cs = cs->next_callee)
> >      cs->count = cs->count.apply_scale (remainder, orig_node_count);
> >
> > 2018-12-07  Bin Cheng  <[hidden email]>
> >
> >         * ipa-cp.c (update_profiling_info): Call adjust_for_ipa_scaling for
> >         zero profile count.