* [PATCH] Guard against applying scale with 0 denominator
@ 2022-05-06 20:31 Eugene Rozenfeld
2022-05-09 7:42 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Eugene Rozenfeld @ 2022-05-06 20:31 UTC (permalink / raw)
To: gcc-patches
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));
free (bbs);
}
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Guard against applying scale with 0 denominator
2022-05-06 20:31 [PATCH] Guard against applying scale with 0 denominator Eugene Rozenfeld
@ 2022-05-09 7:42 ` Richard Biener
2022-05-12 1:37 ` [EXTERNAL] " Eugene Rozenfeld
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2022-05-09 7:42 UTC (permalink / raw)
To: Eugene Rozenfeld, Jan Hubicka; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 denominator
2022-05-09 7:42 ` Richard Biener
@ 2022-05-12 1:37 ` Eugene Rozenfeld
2022-05-12 7:34 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Eugene Rozenfeld @ 2022-05-12 1:37 UTC (permalink / raw)
To: Richard Biener, Jan Hubicka; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 denominator
2022-05-12 1:37 ` [EXTERNAL] " Eugene Rozenfeld
@ 2022-05-12 7:34 ` Richard Biener
2022-05-20 22:28 ` Eugene Rozenfeld
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2022-05-12 7:34 UTC (permalink / raw)
To: Eugene Rozenfeld; +Cc: Jan Hubicka, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 denominator
2022-05-12 7:34 ` Richard Biener
@ 2022-05-20 22:28 ` Eugene Rozenfeld
2022-05-23 10:33 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Eugene Rozenfeld @ 2022-05-20 22:28 UTC (permalink / raw)
To: Richard Biener; +Cc: Jan Hubicka, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 5029 bytes --]
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
[-- Attachment #2: 0001-Fix-profile-count-maintenance-in-vectorizer-peeling.patch --]
[-- Type: application/octet-stream, Size: 3297 bytes --]
From d439b8a5ad0af8ae6b5e0086eed4eebc160fa9aa Mon Sep 17 00:00:00 2001
From: Eugene Rozenfeld <erozen@microsoft.com>
Date: Tue, 26 Apr 2022 14:28:16 -0700
Subject: [PATCH] Fix profile count maintenance in vectorizer peeling.
This patch changes the code to save/restore profile counts for
the epliog loop (when not using scalar loop in the epilog)
instead of scaling them down and then back up, which may lead
to problems if we scale down to 0.
Tested on x86_64-pc-linux-gnu.
gcc/ChangeLog:
* tree-loop-vect-manip.cc (vect_do_peeling): Save/restore profile
counts for the epilog loop.
---
gcc/tree-vect-loop-manip.cc | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
index a7bbc916bbc..501a6ab99f0 100644
--- a/gcc/tree-vect-loop-manip.cc
+++ b/gcc/tree-vect-loop-manip.cc
@@ -2803,10 +2803,21 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1,
if (LOOP_VINFO_PEELING_FOR_GAPS (loop_vinfo))
skip_epilog = false;
+ class loop *scalar_loop = LOOP_VINFO_SCALAR_LOOP (loop_vinfo);
+ auto_vec<profile_count> original_counts;
+ basic_block *original_bbs = NULL;
+
if (skip_vector)
{
split_edge (loop_preheader_edge (loop));
+ if (epilog_peeling && (vect_epilogues || scalar_loop == NULL))
+ {
+ original_bbs = get_loop_body (loop);
+ for (unsigned int i = 0; i < loop->num_nodes; i++)
+ original_counts.safe_push(original_bbs[i]->count);
+ }
+
/* 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
@@ -2821,7 +2832,6 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1,
}
dump_user_location_t loop_loc = find_loop_location (loop);
- class loop *scalar_loop = LOOP_VINFO_SCALAR_LOOP (loop_vinfo);
if (vect_epilogues)
/* Make sure to set the epilogue's epilogue scalar loop, such that we can
use the original scalar loop as remaining epilogue if necessary. */
@@ -2975,16 +2985,19 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1,
a merge point of control flow. */
guard_to->count = guard_bb->count;
- /* Scale probability of epilog loop back.
- FIXME: We should avoid scaling down and back up. Profile may
- 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));
- free (bbs);
+ /* Restore the counts of the epilog loop if we didn't use the scalar loop. */
+ if (vect_epilogues || scalar_loop == NULL)
+ {
+ gcc_assert(epilog->num_nodes == loop->num_nodes);
+ basic_block *bbs = get_loop_body (epilog);
+ for (unsigned int i = 0; i < epilog->num_nodes; i++)
+ {
+ gcc_assert(get_bb_original (bbs[i]) == original_bbs[i]);
+ bbs[i]->count = original_counts[i];
+ }
+ free (bbs);
+ free (original_bbs);
+ }
}
basic_block bb_before_epilog = loop_preheader_edge (epilog)->src;
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 denominator
2022-05-20 22:28 ` Eugene Rozenfeld
@ 2022-05-23 10:33 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2022-05-23 10:33 UTC (permalink / raw)
To: Eugene Rozenfeld; +Cc: Jan Hubicka, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-23 10:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-06 20:31 [PATCH] Guard against applying scale with 0 denominator Eugene Rozenfeld
2022-05-09 7:42 ` Richard Biener
2022-05-12 1:37 ` [EXTERNAL] " Eugene Rozenfeld
2022-05-12 7:34 ` Richard Biener
2022-05-20 22:28 ` Eugene Rozenfeld
2022-05-23 10:33 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).