Thank you for the feedback Richard. I attached a patch that saves/restores counts if the epilog doesn't use a scalar loop.
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
0001-Fix-profile-count-maintenance-in-vectorizer-peeling.patch
Description: 0001-Fix-profile-count-maintenance-in-vectorizer-peeling.patch
