This is the mail archive of the
mailing list for the GCC project.
Re: [patch] Fix node weight updates during ipa-cp (issue7812053)
- From: Teresa Johnson <tejohnson at google dot com>
- To: "H.J. Lu" <hjl dot tools at gmail dot com>
- Cc: Richard Biener <richard dot guenther at gmail dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, reply at codereview dot appspotmail dot com
- Date: Mon, 29 Apr 2013 10:31:28 -0700
- Subject: Re: [patch] Fix node weight updates during ipa-cp (issue7812053)
- References: <20130327172238 dot 545EF80580 at tjsboxrox dot mtv dot corp dot google dot com> <CAFiYyc1F7ncAXzN9eN3wuZzpZvk=mpKM7D4CcBt6hCKpeM1xtA at mail dot gmail dot com> <CAAe5K+WCmr8sqGgFm8vVqDGe5Xfe840jBn7Suf5ZGQkezO38Bg at mail dot gmail dot com> <CAMe9rOrM_f9FXimjDROU5CUDPzbELJ4bzinaoS8oqX=eKh5MdQ at mail dot gmail dot com> <CAAe5K+XcV1MT6wZVNCphuc1Mg+MEmJsOR0yLNyPJ4GTMuA=OUw at mail dot gmail dot com>
FYI, Fixed in r198416.
On Thu, Apr 25, 2013 at 10:19 PM, Teresa Johnson <firstname.lastname@example.org> wrote:
> Reproduced. This looks like another instance of a case I found testing
> my follow-on patch: the helper routines have some assertion checking
> that is too strict for the broader usage where we may be scaling
> counts up and not just down. I am verifying and will send a patch in
> the morning that suppresses this assert, which is the approach I am
> taking in the follow-on patch also coming tomorrow.
> On Thu, Apr 25, 2013 at 3:29 PM, H.J. Lu <email@example.com> wrote:
>> On Fri, Apr 5, 2013 at 7:18 AM, Teresa Johnson <firstname.lastname@example.org> wrote:
>>> On Thu, Mar 28, 2013 at 2:27 AM, Richard Biener
>>> <email@example.com> wrote:
>>>> On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <firstname.lastname@example.org> wrote:
>>>>> I found that the node weight updates on cloned nodes during ipa-cp were
>>>>> leading to incorrect/insane weights. Both the original and new node weight
>>>>> computations used truncating divides, leading to a loss of total node weight.
>>>>> I have fixed this by making both rounding integer divides.
>>>>> Bootstrapped and tested on x86-64-unknown-linux-gnu. Ok for trunk?
>>>> I'm sure we can outline a rounding integer divide inline function on
>>>> gcov_type. To gcov-io.h, I suppose.
>>>> Otherwise this looks ok to me.
>>> Thanks. I went ahead and worked on outlining this functionality. In
>>> the process of doing so, I discovered that there was already a method
>>> in basic-block.h to do part of this: apply_probability(), which does
>>> the rounding divide by REG_BR_PROB_BASE. There is a related function
>>> combine_probabilities() that takes 2 int probabilities instead of a
>>> gcov_type and an int probability. I decided to use apply_probability()
>>> in ipa-cp, and add a new macro GCOV_COMPUTE_SCALE to basic-block.h to
>>> compute the scale factor/probability via a rounding divide. So the
>>> ipa-cp changes I made use both GCOV_COMPUTE_SCALE and
>>> I then went through all the code to look for instances where we were
>>> computing scale factors/probabilities and performing scaling. I found
>>> a mix of existing uses of apply/combine_probabilities, uses of RDIV,
>>> inlined rounding divides, and truncating divides. I think it would be
>>> good to unify all of this. As a first step, I replaced all inline code
>>> sequences that were already doing rounding divides to compute scale
>>> factors/probabilities or do the scaling, to instead use the
>>> appropriate helper function/macro described above. For these
>>> locations, there should be no change to behavior.
>>> There are a number of places where there are truncating divides right
>>> now. Since changing those may impact the resulting behavior, for this
>>> patch I simply added a comment as to which helper they should use. As
>>> soon as this patch goes in I am planning to change those to use the
>>> appropriate helper and test performance, and then will send that patch
>>> for review. So for this patch, the only place where behavior is
>>> changed is in ipa-cp which was my original change.
>>> New patch is attached. Bootstrapped (both bootstrap and
>>> profiledbootstrap) and tested on x86-64-unknown-linux-gnu. Ok for
>> This caused:
> Teresa Johnson | Software Engineer | email@example.com | 408-460-2413
Teresa Johnson | Software Engineer | firstname.lastname@example.org | 408-460-2413