

Created:
7 years, 10 months ago by tejohnson Modified:
6 years, 4 months ago Reviewers:
richard.guenther, hjl.tools, hubicka CC:
gccpatches_gcc.gnu.org Base URL:
svn+ssh://gcc.gnu.org/svn/gcc/trunk/gcc/ Visibility:
Public. 
Patch Set 1 #MessagesTotal messages: 13
I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? 20130327 Teresa Johnson <tejohnson@google.com> * ipacp.c (update_profiling_info): Perform rounding integer division when updating weights instead of truncating. (update_specialized_profile): Ditto. Index: ipacp.c ===================================================================  ipacp.c (revision 197118) +++ ipacp.c (working copy) @@ 2588,14 +2588,18 @@ update_profiling_info (struct cgraph_node *orig_no for (cs = new_node>callees; cs ; cs = cs>next_callee) if (cs>frequency)  cs>count = cs>count * (new_sum * REG_BR_PROB_BASE  / orig_node_count) / REG_BR_PROB_BASE; + cs>count = (cs>count + * ((new_sum * REG_BR_PROB_BASE + orig_node_count/2) + / orig_node_count) + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; else cs>count = 0; for (cs = orig_node>callees; cs ; cs = cs>next_callee)  cs>count = cs>count * (remainder * REG_BR_PROB_BASE  / orig_node_count) / REG_BR_PROB_BASE; + cs>count = (cs>count + * ((remainder * REG_BR_PROB_BASE + orig_node_count/2) + / orig_node_count) + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; if (dump_file) dump_profile_updates (orig_node, new_node); @@ 2627,14 +2631,19 @@ update_specialized_profile (struct cgraph_node *ne for (cs = new_node>callees; cs ; cs = cs>next_callee) if (cs>frequency)  cs>count += cs>count * redirected_sum / new_node_count; + cs>count += (cs>count + * ((redirected_sum * REG_BR_PROB_BASE + + new_node_count/2) / new_node_count) + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; else cs>count = 0; for (cs = orig_node>callees; cs ; cs = cs>next_callee) {  gcov_type dec = cs>count * (redirected_sum * REG_BR_PROB_BASE  / orig_node_count) / REG_BR_PROB_BASE; + gcov_type dec = (cs>count + * ((redirected_sum * REG_BR_PROB_BASE + + orig_node_count/2) / orig_node_count) + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; if (dec < cs>count) cs>count = dec; else  This patch is available for review at http://codereview.appspot.com/7812053
Sign in to reply to this message.
On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: > I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? I'm sure we can outline a rounding integer divide inline function on gcov_type. To gcovio.h, I suppose. Otherwise this looks ok to me. Thanks, Richard. > 20130327 Teresa Johnson <tejohnson@google.com> > > * ipacp.c (update_profiling_info): Perform rounding integer > division when updating weights instead of truncating. > (update_specialized_profile): Ditto. > > Index: ipacp.c > =================================================================== >  ipacp.c (revision 197118) > +++ ipacp.c (working copy) > @@ 2588,14 +2588,18 @@ update_profiling_info (struct cgraph_node *orig_no > > for (cs = new_node>callees; cs ; cs = cs>next_callee) > if (cs>frequency) >  cs>count = cs>count * (new_sum * REG_BR_PROB_BASE >  / orig_node_count) / REG_BR_PROB_BASE; > + cs>count = (cs>count > + * ((new_sum * REG_BR_PROB_BASE + orig_node_count/2) > + / orig_node_count) > + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; > else > cs>count = 0; > > for (cs = orig_node>callees; cs ; cs = cs>next_callee) >  cs>count = cs>count * (remainder * REG_BR_PROB_BASE >  / orig_node_count) / REG_BR_PROB_BASE; > + cs>count = (cs>count > + * ((remainder * REG_BR_PROB_BASE + orig_node_count/2) > + / orig_node_count) > + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; > > if (dump_file) > dump_profile_updates (orig_node, new_node); > @@ 2627,14 +2631,19 @@ update_specialized_profile (struct cgraph_node *ne > > for (cs = new_node>callees; cs ; cs = cs>next_callee) > if (cs>frequency) >  cs>count += cs>count * redirected_sum / new_node_count; > + cs>count += (cs>count > + * ((redirected_sum * REG_BR_PROB_BASE > + + new_node_count/2) / new_node_count) > + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; > else > cs>count = 0; > > for (cs = orig_node>callees; cs ; cs = cs>next_callee) > { >  gcov_type dec = cs>count * (redirected_sum * REG_BR_PROB_BASE >  / orig_node_count) / REG_BR_PROB_BASE; > + gcov_type dec = (cs>count > + * ((redirected_sum * REG_BR_PROB_BASE > + + orig_node_count/2) / orig_node_count) > + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; > if (dec < cs>count) > cs>count = dec; > else > >  > This patch is available for review at http://codereview.appspot.com/7812053
Sign in to reply to this message.
On Thu, Mar 28, 2013 at 2:27 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: >> I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? > > I'm sure we can outline a rounding integer divide inline function on > gcov_type. To gcovio.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 basicblock.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 ipacp, and add a new macro GCOV_COMPUTE_SCALE to basicblock.h to compute the scale factor/probability via a rounding divide. So the ipacp changes I made use both GCOV_COMPUTE_SCALE and apply_probability. 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 ipacp which was my original change. New patch is attached. Bootstrapped (both bootstrap and profiledbootstrap) and tested on x8664unknownlinuxgnu. Ok for trunk? Thanks, Teresa > > Thanks, > Richard. > >> 20130327 Teresa Johnson <tejohnson@google.com> >> >> * ipacp.c (update_profiling_info): Perform rounding integer >> division when updating weights instead of truncating. >> (update_specialized_profile): Ditto. >> >> Index: ipacp.c >> =================================================================== >>  ipacp.c (revision 197118) >> +++ ipacp.c (working copy) >> @@ 2588,14 +2588,18 @@ update_profiling_info (struct cgraph_node *orig_no >> >> for (cs = new_node>callees; cs ; cs = cs>next_callee) >> if (cs>frequency) >>  cs>count = cs>count * (new_sum * REG_BR_PROB_BASE >>  / orig_node_count) / REG_BR_PROB_BASE; >> + cs>count = (cs>count >> + * ((new_sum * REG_BR_PROB_BASE + orig_node_count/2) >> + / orig_node_count) >> + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; >> else >> cs>count = 0; >> >> for (cs = orig_node>callees; cs ; cs = cs>next_callee) >>  cs>count = cs>count * (remainder * REG_BR_PROB_BASE >>  / orig_node_count) / REG_BR_PROB_BASE; >> + cs>count = (cs>count >> + * ((remainder * REG_BR_PROB_BASE + orig_node_count/2) >> + / orig_node_count) >> + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; >> >> if (dump_file) >> dump_profile_updates (orig_node, new_node); >> @@ 2627,14 +2631,19 @@ update_specialized_profile (struct cgraph_node *ne >> >> for (cs = new_node>callees; cs ; cs = cs>next_callee) >> if (cs>frequency) >>  cs>count += cs>count * redirected_sum / new_node_count; >> + cs>count += (cs>count >> + * ((redirected_sum * REG_BR_PROB_BASE >> + + new_node_count/2) / new_node_count) >> + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; >> else >> cs>count = 0; >> >> for (cs = orig_node>callees; cs ; cs = cs>next_callee) >> { >>  gcov_type dec = cs>count * (redirected_sum * REG_BR_PROB_BASE >>  / orig_node_count) / REG_BR_PROB_BASE; >> + gcov_type dec = (cs>count >> + * ((redirected_sum * REG_BR_PROB_BASE >> + + orig_node_count/2) / orig_node_count) >> + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; >> if (dec < cs>count) >> cs>count = dec; >> else >> >>  >> This patch is available for review at http://codereview.appspot.com/7812053  Teresa Johnson  Software Engineer  tejohnson@google.com  4084602413
Sign in to reply to this message.
On Fri, Apr 5, 2013 at 4:18 PM, Teresa Johnson <tejohnson@google.com> wrote: > On Thu, Mar 28, 2013 at 2:27 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: >>> I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? >> >> I'm sure we can outline a rounding integer divide inline function on >> gcov_type. To gcovio.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 basicblock.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 ipacp, and add a new macro GCOV_COMPUTE_SCALE to basicblock.h to > compute the scale factor/probability via a rounding divide. So the > ipacp changes I made use both GCOV_COMPUTE_SCALE and > apply_probability. > > 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 ipacp which was my original change. > > New patch is attached. Bootstrapped (both bootstrap and > profiledbootstrap) and tested on x8664unknownlinuxgnu. Ok for > trunk? Ok. Thanks, Richard. > Thanks, > Teresa > >> >> Thanks, >> Richard. >> >>> 20130327 Teresa Johnson <tejohnson@google.com> >>> >>> * ipacp.c (update_profiling_info): Perform rounding integer >>> division when updating weights instead of truncating. >>> (update_specialized_profile): Ditto. >>> >>> Index: ipacp.c >>> =================================================================== >>>  ipacp.c (revision 197118) >>> +++ ipacp.c (working copy) >>> @@ 2588,14 +2588,18 @@ update_profiling_info (struct cgraph_node *orig_no >>> >>> for (cs = new_node>callees; cs ; cs = cs>next_callee) >>> if (cs>frequency) >>>  cs>count = cs>count * (new_sum * REG_BR_PROB_BASE >>>  / orig_node_count) / REG_BR_PROB_BASE; >>> + cs>count = (cs>count >>> + * ((new_sum * REG_BR_PROB_BASE + orig_node_count/2) >>> + / orig_node_count) >>> + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; >>> else >>> cs>count = 0; >>> >>> for (cs = orig_node>callees; cs ; cs = cs>next_callee) >>>  cs>count = cs>count * (remainder * REG_BR_PROB_BASE >>>  / orig_node_count) / REG_BR_PROB_BASE; >>> + cs>count = (cs>count >>> + * ((remainder * REG_BR_PROB_BASE + orig_node_count/2) >>> + / orig_node_count) >>> + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; >>> >>> if (dump_file) >>> dump_profile_updates (orig_node, new_node); >>> @@ 2627,14 +2631,19 @@ update_specialized_profile (struct cgraph_node *ne >>> >>> for (cs = new_node>callees; cs ; cs = cs>next_callee) >>> if (cs>frequency) >>>  cs>count += cs>count * redirected_sum / new_node_count; >>> + cs>count += (cs>count >>> + * ((redirected_sum * REG_BR_PROB_BASE >>> + + new_node_count/2) / new_node_count) >>> + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; >>> else >>> cs>count = 0; >>> >>> for (cs = orig_node>callees; cs ; cs = cs>next_callee) >>> { >>>  gcov_type dec = cs>count * (redirected_sum * REG_BR_PROB_BASE >>>  / orig_node_count) / REG_BR_PROB_BASE; >>> + gcov_type dec = (cs>count >>> + * ((redirected_sum * REG_BR_PROB_BASE >>> + + orig_node_count/2) / orig_node_count) >>> + + REG_BR_PROB_BASE/2) / REG_BR_PROB_BASE; >>> if (dec < cs>count) >>> cs>count = dec; >>> else >>> >>>  >>> This patch is available for review at http://codereview.appspot.com/7812053 > > > >  > Teresa Johnson  Software Engineer  tejohnson@google.com  4084602413
Sign in to reply to this message.
> On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: > > I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? > > I'm sure we can outline a rounding integer divide inline function on > gcov_type. To gcovio.h, I suppose. Most of code currently use RDIV for rounding divides (at lest I am slowly trying to migrate to that). Honza
Sign in to reply to this message.
Hi Honza, I converted all other weight update locations to use the helper functions in basicblock.h instead of truncation (the patch I checked in a couple weeks ago covered the cases that already used RDIV  see the followon messages in this thread). I am almost done testing with SPEC cpu2006. So far I haven't seen any performance effects, so I am hoping to send this for review today or tomorrow. Thanks, Teresa On Mon, Apr 22, 2013 at 10:27 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >> On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: >> > I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? >> >> I'm sure we can outline a rounding integer divide inline function on >> gcov_type. To gcovio.h, I suppose. > > Most of code currently use RDIV for rounding divides (at lest I am slowly trying > to migrate to that). > > Honza  Teresa Johnson  Software Engineer  tejohnson@google.com  4084602413
Sign in to reply to this message.
On Fri, Apr 5, 2013 at 7:18 AM, Teresa Johnson <tejohnson@google.com> wrote: > On Thu, Mar 28, 2013 at 2:27 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: >>> I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? >> >> I'm sure we can outline a rounding integer divide inline function on >> gcov_type. To gcovio.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 basicblock.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 ipacp, and add a new macro GCOV_COMPUTE_SCALE to basicblock.h to > compute the scale factor/probability via a rounding divide. So the > ipacp changes I made use both GCOV_COMPUTE_SCALE and > apply_probability. > > 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 ipacp which was my original change. > > New patch is attached. Bootstrapped (both bootstrap and > profiledbootstrap) and tested on x8664unknownlinuxgnu. Ok for > trunk? > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57077 H.J.
Sign in to reply to this message.
I'll take a look. Teresa On Thu, Apr 25, 2013 at 3:29 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Fri, Apr 5, 2013 at 7:18 AM, Teresa Johnson <tejohnson@google.com> wrote: >> On Thu, Mar 28, 2013 at 2:27 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>> I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? >>> >>> I'm sure we can outline a rounding integer divide inline function on >>> gcov_type. To gcovio.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 basicblock.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 ipacp, and add a new macro GCOV_COMPUTE_SCALE to basicblock.h to >> compute the scale factor/probability via a rounding divide. So the >> ipacp changes I made use both GCOV_COMPUTE_SCALE and >> apply_probability. >> >> 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 ipacp which was my original change. >> >> New patch is attached. Bootstrapped (both bootstrap and >> profiledbootstrap) and tested on x8664unknownlinuxgnu. Ok for >> trunk? >> > > This caused: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57077 > > > H.J.  Teresa Johnson  Software Engineer  tejohnson@google.com  4084602413
Sign in to reply to this message.
Reproduced. This looks like another instance of a case I found testing my followon 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 followon patch also coming tomorrow. Teresa On Thu, Apr 25, 2013 at 3:29 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Fri, Apr 5, 2013 at 7:18 AM, Teresa Johnson <tejohnson@google.com> wrote: >> On Thu, Mar 28, 2013 at 2:27 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>> I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? >>> >>> I'm sure we can outline a rounding integer divide inline function on >>> gcov_type. To gcovio.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 basicblock.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 ipacp, and add a new macro GCOV_COMPUTE_SCALE to basicblock.h to >> compute the scale factor/probability via a rounding divide. So the >> ipacp changes I made use both GCOV_COMPUTE_SCALE and >> apply_probability. >> >> 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 ipacp which was my original change. >> >> New patch is attached. Bootstrapped (both bootstrap and >> profiledbootstrap) and tested on x8664unknownlinuxgnu. Ok for >> trunk? >> > > This caused: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57077 > > > H.J.  Teresa Johnson  Software Engineer  tejohnson@google.com  4084602413
Sign in to reply to this message.
FYI, Fixed in r198416. Thanks, Teresa On Thu, Apr 25, 2013 at 10:19 PM, Teresa Johnson <tejohnson@google.com> wrote: > Reproduced. This looks like another instance of a case I found testing > my followon 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 followon patch also coming tomorrow. > > Teresa > > On Thu, Apr 25, 2013 at 3:29 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Fri, Apr 5, 2013 at 7:18 AM, Teresa Johnson <tejohnson@google.com> wrote: >>> On Thu, Mar 28, 2013 at 2:27 AM, Richard Biener >>> <richard.guenther@gmail.com> wrote: >>>> On Wed, Mar 27, 2013 at 6:22 PM, Teresa Johnson <tejohnson@google.com> wrote: >>>>> I found that the node weight updates on cloned nodes during ipacp 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 x8664unknownlinuxgnu. Ok for trunk? >>>> >>>> I'm sure we can outline a rounding integer divide inline function on >>>> gcov_type. To gcovio.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 basicblock.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 ipacp, and add a new macro GCOV_COMPUTE_SCALE to basicblock.h to >>> compute the scale factor/probability via a rounding divide. So the >>> ipacp changes I made use both GCOV_COMPUTE_SCALE and >>> apply_probability. >>> >>> 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 ipacp which was my original change. >>> >>> New patch is attached. Bootstrapped (both bootstrap and >>> profiledbootstrap) and tested on x8664unknownlinuxgnu. Ok for >>> trunk? >>> >> >> This caused: >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57077 >> >> >> H.J. > > > >  > Teresa Johnson  Software Engineer  tejohnson@google.com  4084602413  Teresa Johnson  Software Engineer  tejohnson@google.com  4084602413
Sign in to reply to this message.
On Mon, Apr 29, 2013 at 10:31 AM, Teresa Johnson <tejohnson@google.com> wrote: > FYI, Fixed in r198416. > > Thanks, > Teresa > I noticed that sometimes GCC generates: _8 = memcpy (ret_6, s_2(D), len_4); _8 = memcpy (ret_6, s_2(D), len_4); memcpy (_17, buffer_12(D), add_16); memcpy (_17, buffer_12(D), add_16); memcpy (_25, _28, _27); memcpy (_25, _28, _27); memcpy (_39, buffer_2, len_4); memcpy (_39, buffer_2, len_4); memcpy (_16, &fillbuf, pad_1); memcpy (_16, &fillbuf, pad_1); ...  H.J.
Sign in to reply to this message.
On Mon, Apr 29, 2013 at 12:35 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Apr 29, 2013 at 10:31 AM, Teresa Johnson <tejohnson@google.com> wrote: >> FYI, Fixed in r198416. >> >> Thanks, >> Teresa >> > > I noticed that sometimes GCC generates: > > _8 = memcpy (ret_6, s_2(D), len_4); > _8 = memcpy (ret_6, s_2(D), len_4); > memcpy (_17, buffer_12(D), add_16); > memcpy (_17, buffer_12(D), add_16); > memcpy (_25, _28, _27); > memcpy (_25, _28, _27); > memcpy (_39, buffer_2, len_4); > memcpy (_39, buffer_2, len_4); > memcpy (_16, &fillbuf, pad_1); > memcpy (_16, &fillbuf, pad_1); I am getting this too with a profiledbootstrap with LTO. However, this isn't due to my changes. I had confirmed that after reverting my changes (r197595 and now the followon fix r198416) this problem still occurs. Teresa > ... > > > >  > H.J.  Teresa Johnson  Software Engineer  tejohnson@google.com  4084602413
Sign in to reply to this message.
On Tue, Apr 30, 2013 at 7:02 AM, Teresa Johnson <tejohnson@google.com> wrote: > On Mon, Apr 29, 2013 at 12:35 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Mon, Apr 29, 2013 at 10:31 AM, Teresa Johnson <tejohnson@google.com> wrote: >>> FYI, Fixed in r198416. >>> >>> Thanks, >>> Teresa >>> >> >> I noticed that sometimes GCC generates: >> >> _8 = memcpy (ret_6, s_2(D), len_4); >> _8 = memcpy (ret_6, s_2(D), len_4); >> memcpy (_17, buffer_12(D), add_16); >> memcpy (_17, buffer_12(D), add_16); >> memcpy (_25, _28, _27); >> memcpy (_25, _28, _27); >> memcpy (_39, buffer_2, len_4); >> memcpy (_39, buffer_2, len_4); >> memcpy (_16, &fillbuf, pad_1); >> memcpy (_16, &fillbuf, pad_1); > > I am getting this too with a profiledbootstrap with LTO. However, this > isn't due to my changes. I had confirmed that after reverting my > changes (r197595 and now the followon fix r198416) this problem still > occurs. > > Teresa There is a strayed debug_gimple_stmt. I am checking in this patch to fix it.  H.J.  diff git a/gcc/valueprof.c b/gcc/valueprof.c index 3348d7f..b665b1c 100644  a/gcc/valueprof.c +++ b/gcc/valueprof.c @@ 416,7 +416,6 @@ stream_in_histogram_value (struct lto_input_block *ib, gimple stmt) new_val>n_counters = ncounters; for (i = 0; i < ncounters; i++) new_val>hvalue.counters[i] = streamer_read_gcov_count (ib);  debug_gimple_stmt (stmt); if (!next_p) gimple_add_histogram_value (cfun, stmt, new_val); else
Sign in to reply to this message.
