Fix incorrect loop exit edge probability [PR103270]

Message ID 20211123055145.475825-1-luoxhu@linux.ibm.com
State New
Headers
Series Fix incorrect loop exit edge probability [PR103270] |

Commit Message

Xionghu Luo Nov. 23, 2021, 5:51 a.m. UTC
  r12-4526 cancelled jump thread path rotates loop. It exposes a issue in
profile-estimate when predict_extra_loop_exits, outer loop's exit edge
is marked as inner loop's extra loop exit and set with incorrect
prediction, then a hot inner loop will become cold loop finally through
optimizations, this patch ignores the EDGE_DFS_BACK edge when searching
extra exit edges to avoid unexpected predict_edge.

gcc/ChangeLog:

	PR middle-end/103270
	* predict.c (predict_extra_loop_exits): Ignore EDGE_DFS_BACK edge.

gcc/ChangeLog:

	PR middle-end/103270
	* predict.c (predict_extra_loop_exits): New.
---
 gcc/predict.c | 4 ++++
 1 file changed, 4 insertions(+)
  

Comments

Xionghu Luo Nov. 23, 2021, 5:54 a.m. UTC | #1
On 2021/11/23 13:51, Xionghu Luo wrote:
> r12-4526 cancelled jump thread path rotates loop. It exposes a issue in
> profile-estimate when predict_extra_loop_exits, outer loop's exit edge
> is marked as inner loop's extra loop exit and set with incorrect
> prediction, then a hot inner loop will become cold loop finally through
> optimizations, this patch ignores the EDGE_DFS_BACK edge when searching
> extra exit edges to avoid unexpected predict_edge.
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/103270
> 	* predict.c (predict_extra_loop_exits): Ignore EDGE_DFS_BACK edge.
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/103270
> 	* predict.c (predict_extra_loop_exits): New.
> ---
>  gcc/predict.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gcc/predict.c b/gcc/predict.c
> index 68b11135680..1ae8ccff72c 100644
> --- a/gcc/predict.c
> +++ b/gcc/predict.c
> @@ -1910,6 +1910,10 @@ predict_extra_loop_exits (edge exit_edge)
>  	continue;
>        if ((check_value_one ^ integer_onep (val)) == 1)
>  	continue;
> +      if (e->flags & EDGE_DFS_BACK)
> +	continue;

Sorry, made a mistake before send the patch, #if 0 #endif should be
removed...


>        if (EDGE_COUNT (e->src->succs) != 1)
>  	{
>  	  predict_paths_leading_to_edge (e, PRED_LOOP_EXTRA_EXIT, NOT_TAKEN);
>
  
Richard Biener Nov. 23, 2021, 9:42 a.m. UTC | #2
On Tue, Nov 23, 2021 at 6:52 AM Xionghu Luo <luoxhu@linux.ibm.com> wrote:
>
> r12-4526 cancelled jump thread path rotates loop. It exposes a issue in
> profile-estimate when predict_extra_loop_exits, outer loop's exit edge
> is marked as inner loop's extra loop exit and set with incorrect
> prediction, then a hot inner loop will become cold loop finally through
> optimizations, this patch ignores the EDGE_DFS_BACK edge when searching
> extra exit edges to avoid unexpected predict_edge.

Not sure how outer vs. inner loop exit correlates with EDGE_DFS_BACK,
I have expected a check based on which loop is exited by the edge instead?
A backedge should never be an exit, no?

Note that the profile pass does not yet mark backedges so EDGE_DFS_BACK
settings are unreliable.

Richard.

>
> gcc/ChangeLog:
>
>         PR middle-end/103270
>         * predict.c (predict_extra_loop_exits): Ignore EDGE_DFS_BACK edge.
>
> gcc/ChangeLog:
>
>         PR middle-end/103270
>         * predict.c (predict_extra_loop_exits): New.
> ---
>  gcc/predict.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/gcc/predict.c b/gcc/predict.c
> index 68b11135680..1ae8ccff72c 100644
> --- a/gcc/predict.c
> +++ b/gcc/predict.c
> @@ -1910,6 +1910,10 @@ predict_extra_loop_exits (edge exit_edge)
>         continue;
>        if ((check_value_one ^ integer_onep (val)) == 1)
>         continue;
> +#if 0
> +      if (e->flags & EDGE_DFS_BACK)
> +       continue;
> +#endif
>        if (EDGE_COUNT (e->src->succs) != 1)
>         {
>           predict_paths_leading_to_edge (e, PRED_LOOP_EXTRA_EXIT, NOT_TAKEN);
> --
> 2.25.1
>
  
Jan Hubicka Nov. 23, 2021, 9:50 a.m. UTC | #3
> On Tue, Nov 23, 2021 at 6:52 AM Xionghu Luo <luoxhu@linux.ibm.com> wrote:
> >
> > r12-4526 cancelled jump thread path rotates loop. It exposes a issue in
> > profile-estimate when predict_extra_loop_exits, outer loop's exit edge
> > is marked as inner loop's extra loop exit and set with incorrect
> > prediction, then a hot inner loop will become cold loop finally through
> > optimizations, this patch ignores the EDGE_DFS_BACK edge when searching
> > extra exit edges to avoid unexpected predict_edge.
> 
> Not sure how outer vs. inner loop exit correlates with EDGE_DFS_BACK,
> I have expected a check based on which loop is exited by the edge instead?
> A backedge should never be an exit, no?
> 
> Note that the profile pass does not yet mark backedges so EDGE_DFS_BACK
> settings are unreliable.

So we have two nested loops and an exit which goes from inner loop and
exists both loops.  While processing outer loop we set pretty high exit
probability that is not good for inner loop?

I guess we could just check if exit edge source basic block has same
loop depth as the loop we ar eprocesing?

Honza
> 
> Richard.
> 
> >
> > gcc/ChangeLog:
> >
> >         PR middle-end/103270
> >         * predict.c (predict_extra_loop_exits): Ignore EDGE_DFS_BACK edge.
> >
> > gcc/ChangeLog:
> >
> >         PR middle-end/103270
> >         * predict.c (predict_extra_loop_exits): New.
> > ---
> >  gcc/predict.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/gcc/predict.c b/gcc/predict.c
> > index 68b11135680..1ae8ccff72c 100644
> > --- a/gcc/predict.c
> > +++ b/gcc/predict.c
> > @@ -1910,6 +1910,10 @@ predict_extra_loop_exits (edge exit_edge)
> >         continue;
> >        if ((check_value_one ^ integer_onep (val)) == 1)
> >         continue;
> > +#if 0
> > +      if (e->flags & EDGE_DFS_BACK)
> > +       continue;
> > +#endif
> >        if (EDGE_COUNT (e->src->succs) != 1)
> >         {
> >           predict_paths_leading_to_edge (e, PRED_LOOP_EXTRA_EXIT, NOT_TAKEN);
> > --
> > 2.25.1
> >
  

Patch

diff --git a/gcc/predict.c b/gcc/predict.c
index 68b11135680..1ae8ccff72c 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -1910,6 +1910,10 @@  predict_extra_loop_exits (edge exit_edge)
 	continue;
       if ((check_value_one ^ integer_onep (val)) == 1)
 	continue;
+#if 0
+      if (e->flags & EDGE_DFS_BACK)
+	continue;
+#endif
       if (EDGE_COUNT (e->src->succs) != 1)
 	{
 	  predict_paths_leading_to_edge (e, PRED_LOOP_EXTRA_EXIT, NOT_TAKEN);