On Sat, May 21, 2022 at 12:28 AM Eugene Rozenfeld
<eugene.rozenf...@microsoft.com> wrote:
>
> Thank you for the feedback Richard. I attached a patch that saves/restores
> counts if the epilog doesn't use a scalar loop.
OK.
Thanks,
Richard.
> Eugene
>
> -----Original Message-----
> From: Richard Biener <richard.guent...@gmail.com>
> Sent: Thursday, May 12, 2022 12:34 AM
> To: Eugene Rozenfeld <eugene.rozenf...@microsoft.com>
> Cc: Jan Hubicka <hubi...@ucw.cz>; gcc-patches@gcc.gnu.org
> Subject: Re: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0
> denominator
>
> On Thu, May 12, 2022 at 3:37 AM Eugene Rozenfeld
> <eugene.rozenf...@microsoft.com> wrote:
> >
> > In my case this is not exactly what the FIXME in the comment above
> > says. The count is 0 even before the initial scaling happens. I hit this
> > case with some changes I'm working on to enable per-instruction
> > discriminators for AutoFDO.
> >
> > I looked into saving/restoring the old counts but it would be awkward to
> > do. The initial scaling happens here:
> >
> > if (skip_vector)
> > {
> > split_edge (loop_preheader_edge (loop));
> >
> > /* Due to the order in which we peel prolog and epilog, we first
> > propagate probability to the whole loop. The purpose is to
> > avoid adjusting probabilities of both prolog and vector loops
> > separately. Note in this case, the probability of epilog loop
> > needs to be scaled back later. */
> > basic_block bb_before_loop = loop_preheader_edge (loop)->src;
> > if (prob_vector.initialized_p ())
> > {
> > scale_bbs_frequencies (&bb_before_loop, 1, prob_vector);
> > scale_loop_profile (loop, prob_vector, 0);
> > }
> > }
> >
> > The scaling happens before we do the cloning for the epilog so to
> > save/restore the counts we would need to maintain a mapping between the
> > original basic blocks and the cloned basic blocks in the epilog.
>
> There is one already - after the epilogue is copied you can use
> get_bb_original (epilouge_bb) to get at the block it was copied from.
> It could also be that we can rely on the basic-block order to be preserved
> between the copies (I _think_ that would work ... but then assert () for this
> using get_bb_original ()). That means the simplest fix would be to have an
> auto_vec<count> and initialize that from the BB counts in loop body order (we
> also have exactly two BBs in all peeled loops ...
>
> But note we only scaled the scalar if-converted loop but eventually used the
> not if-coverted copy for the epilogue (if not vectorizing the epilogue), so
> indiscriminate scaling back looks wrong in some cases (I'd have to
> double-check the details here).
>
> > I'd like to get my simple fix in since it makes things better even if
> > it doesn't address the issue mentioned In the FIXME.
>
> But don't you need to check that
> bbs[i]->count.apply_probability (prob_vector) is not zero instead of checking
> that bbs[i].count is not zero?
>
> Richard.
>
> > -----Original Message-----
> > From: Richard Biener <richard.guent...@gmail.com>
> > Sent: Monday, May 09, 2022 12:42 AM
> > To: Eugene Rozenfeld <eugene.rozenf...@microsoft.com>; Jan Hubicka
> > <hubi...@ucw.cz>
> > Cc: gcc-patches@gcc.gnu.org
> > Subject: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0
> > denominator
> >
> > On Fri, May 6, 2022 at 10:32 PM Eugene Rozenfeld via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > Calling count.apply_scale with a 0 denominator causes an assert.
> > > This change guards against that.
> > >
> > > Tested on x86_64-pc-linux-gnu.
> > >
> > > gcc/ChangeLog:
> > > * tree-loop-vect-manip.cc (vect_do_peeling): Guard against
> > > applying scale with 0 denominator.
> > > ---
> > > gcc/tree-vect-loop-manip.cc | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/gcc/tree-vect-loop-manip.cc
> > > b/gcc/tree-vect-loop-manip.cc index 1d4337eb261..db54ae69e45 100644
> > > --- a/gcc/tree-vect-loop-manip.cc
> > > +++ b/gcc/tree-vect-loop-manip.cc
> > > @@ -2989,10 +2989,11 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree
> > > niters, tree nitersm1,
> > > get lost if we scale down to 0. */
> > > basic_block *bbs = get_loop_body (epilog);
> > > for (unsigned int i = 0; i < epilog->num_nodes; i++)
> > > - bbs[i]->count = bbs[i]->count.apply_scale
> > > - (bbs[i]->count,
> > > - bbs[i]->count.apply_probability
> > > - (prob_vector));
> > > + if (bbs[i]->count.nonzero_p ())
> > > + bbs[i]->count = bbs[i]->count.apply_scale
> > > + (bbs[i]->count,
> > > + bbs[i]->count.apply_probability
> > > + (prob_vector));
> >
> > So exactly what the FIXME in the comment above says happens. It
> > might be better
> > to save/restore the old counts if the intent is to get them back. I'm not
> > exactly sure where the other scaling happens though.
> >
> > Richard.
> >
> >
> >
> > > free (bbs);
> > > }
> > >
> > > --
> > > 2.25.1
