Guard against applying scale with 0 denominator

  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(-)

--
2.25.1
  

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
  

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.

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.

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Monday, May 09, 2022 12:42 AM
To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; Jan Hubicka <hubicka@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
  

On Thu, May 12, 2022 at 3:37 AM Eugene Rozenfeld
<Eugene.Rozenfeld@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.guenther@gmail.com>
> Sent: Monday, May 09, 2022 12:42 AM
> To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; Jan Hubicka <hubicka@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
  

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.guenther@gmail.com> 
Sent: Thursday, May 12, 2022 12:34 AM
To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>
Cc: Jan Hubicka <hubicka@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.Rozenfeld@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.guenther@gmail.com>
> Sent: Monday, May 09, 2022 12:42 AM
> To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; Jan Hubicka 
> <hubicka@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
  

On Sat, May 21, 2022 at 12:28 AM Eugene Rozenfeld
<Eugene.Rozenfeld@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.guenther@gmail.com>
> Sent: Thursday, May 12, 2022 12:34 AM
> To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>
> Cc: Jan Hubicka <hubicka@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.Rozenfeld@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.guenther@gmail.com>
> > Sent: Monday, May 09, 2022 12:42 AM
> > To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; Jan Hubicka
> > <hubicka@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