Do not combine PRED_LOOP_GUARD and PRED_LOOP_GUARD_WITH_RECURSION

Do not combine PRED_LOOP_GUARD and PRED_LOOP_GUARD_WITH_RECURSION

[Jan Hubicka]

Hi,
this patch avoids both PRED_LOOP_GUARD and PRED_LOOP_GUARD_WITH_RECURSION to be
attached to one edge.  We have logic that prevents same predictor to apply to
one edge twice, but since we split LOOP_GUARD to two more specialized cases,
this no longer fires.

Double prediction happens in exchange benchmark and leads to unrealistically
low hitrates on some edges which in turn leads to bad IPA profile and misguides
ipa-cp.

Unforutnately it seems that the bad profile also leads to bit better
performance by disabling some of loop stuff, but that really ought to be done
in some meaningful way, not by an accident.

Bootstrapped/regtested x86_64-linux, comitted.

Honza

gcc/ChangeLog:

2020-08-11  Jan Hubicka  <hubicka@ucw.cz>

	* predict.c (not_loop_guard_equal_edge_p): New function.
	(maybe_predict_edge): New function.
	(predict_paths_for_bb): Use it.
	(predict_paths_leading_to_edge): Use it.

gcc/testsuite/ChangeLog:

2020-08-11  Jan Hubicka  <hubicka@ucw.cz>

	* gcc.dg/ipa/ipa-clone-2.c: Lower threshold from 500 to 400.

diff --git a/gcc/predict.c b/gcc/predict.c
index 2164a06e083..4c4bba54939 100644
--- a/gcc/predict.c
+++ b/gcc/predict.c
@@ -3122,6 +3122,35 @@ tree_guess_outgoing_edge_probabilities (basic_block bb)
   bb_predictions = NULL;
 }
 \f
+/* Filter function predicate that returns true for a edge predicate P
+   if its edge is equal to DATA.  */
+
+static bool
+not_loop_guard_equal_edge_p (edge_prediction *p, void *data)
+{
+  return p->ep_edge != (edge)data || p->ep_predictor != PRED_LOOP_GUARD;
+}
+
+/* Predict edge E with PRED unless it is already predicted by some predictor
+   considered equivalent.  */
+
+static void
+maybe_predict_edge (edge e, enum br_predictor pred, enum prediction taken)
+{
+  if (edge_predicted_by_p (e, pred, taken))
+    return;
+  if (pred == PRED_LOOP_GUARD
+      && edge_predicted_by_p (e, PRED_LOOP_GUARD_WITH_RECURSION, taken))
+    return;
+  /* Consider PRED_LOOP_GUARD_WITH_RECURSION superrior to LOOP_GUARD.  */
+  if (pred == PRED_LOOP_GUARD_WITH_RECURSION)
+    {
+      edge_prediction **preds = bb_predictions->get (e->src);
+      if (preds)
+	filter_predictions (preds, not_loop_guard_equal_edge_p, e);
+    }
+  predict_edge_def (e, pred, taken);
+}
 /* Predict edges to successors of CUR whose sources are not postdominated by
    BB by PRED and recurse to all postdominators.  */
 
@@ -3177,10 +3206,7 @@ predict_paths_for_bb (basic_block cur, basic_block bb,
 	 regions that are only reachable by abnormal edges.  We simply
 	 prevent visiting given BB twice.  */
       if (found)
-	{
-	  if (!edge_predicted_by_p (e, pred, taken))
-            predict_edge_def (e, pred, taken);
-	}
+	maybe_predict_edge (e, pred, taken);
       else if (bitmap_set_bit (visited, e->src->index))
 	predict_paths_for_bb (e->src, e->src, pred, taken, visited, in_loop);
     }
@@ -3223,7 +3249,7 @@ predict_paths_leading_to_edge (edge e, enum br_predictor pred,
   if (!has_nonloop_edge)
     predict_paths_for_bb (bb, bb, pred, taken, auto_bitmap (), in_loop);
   else
-    predict_edge_def (e, pred, taken);
+    maybe_predict_edge (e, pred, taken);
 }
 \f
 /* This is used to carry information about basic blocks.  It is
diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-clone-2.c b/gcc/testsuite/gcc.dg/ipa/ipa-clone-2.c
index d513020ee8b..53ae25a1e24 100644
--- a/gcc/testsuite/gcc.dg/ipa/ipa-clone-2.c
+++ b/gcc/testsuite/gcc.dg/ipa/ipa-clone-2.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O3 -fdump-ipa-cp-details -fno-early-inlining --param ipa-cp-max-recursive-depth=8" } */
+/* { dg-options "-O3 -fdump-ipa-cp-details -fno-early-inlining --param ipa-cp-max-recursive-depth=8 --param=ipa-cp-eval-threshold=400" } */
 
 int fn();
 

RE: Do not combine PRED_LOOP_GUARD and PRED_LOOP_GUARD_WITH_RECURSION

[Tamar Christina]

Hi Honza,

> -----Original Message-----
> From: Gcc-patches <gcc-patches-bounces@gcc.gnu.org> On Behalf Of Jan
> Hubicka
> Sent: Tuesday, August 11, 2020 11:04 AM
> To: gcc-patches@gcc.gnu.org
> Subject: Do not combine PRED_LOOP_GUARD and
> PRED_LOOP_GUARD_WITH_RECURSION
> 
> Hi,
> this patch avoids both PRED_LOOP_GUARD and
> PRED_LOOP_GUARD_WITH_RECURSION to be attached to one edge.  We
> have logic that prevents same predictor to apply to one edge twice, but since
> we split LOOP_GUARD to two more specialized cases, this no longer fires.
> 
> Double prediction happens in exchange benchmark and leads to
> unrealistically low hitrates on some edges which in turn leads to bad IPA
> profile and misguides ipa-cp.
> 
> Unforutnately it seems that the bad profile also leads to bit better
> performance by disabling some of loop stuff, but that really ought to be done
> in some meaningful way, not by an accident.

Hmm the regression on exchange2 is 11%. Or 1.22% on specint 2017 overall..
This is a rather big regression.. What would be the correct way to do this?

Kind Regards,
Tamar

> 
> Bootstrapped/regtested x86_64-linux, comitted.
> 
> Honza
> 
> gcc/ChangeLog:
> 
> 2020-08-11  Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* predict.c (not_loop_guard_equal_edge_p): New function.
> 	(maybe_predict_edge): New function.
> 	(predict_paths_for_bb): Use it.
> 	(predict_paths_leading_to_edge): Use it.
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-08-11  Jan Hubicka  <hubicka@ucw.cz>
> 
> 	* gcc.dg/ipa/ipa-clone-2.c: Lower threshold from 500 to 400.
> 
> diff --git a/gcc/predict.c b/gcc/predict.c index 2164a06e083..4c4bba54939
> 100644
> --- a/gcc/predict.c
> +++ b/gcc/predict.c
> @@ -3122,6 +3122,35 @@ tree_guess_outgoing_edge_probabilities
> (basic_block bb)
>    bb_predictions = NULL;
>  }
> 
> 
> 
> +/* Filter function predicate that returns true for a edge predicate P
> +   if its edge is equal to DATA.  */
> +
> +static bool
> +not_loop_guard_equal_edge_p (edge_prediction *p, void *data) {
> +  return p->ep_edge != (edge)data || p->ep_predictor !=
> +PRED_LOOP_GUARD; }
> +
> +/* Predict edge E with PRED unless it is already predicted by some predictor
> +   considered equivalent.  */
> +
> +static void
> +maybe_predict_edge (edge e, enum br_predictor pred, enum prediction
> +taken) {
> +  if (edge_predicted_by_p (e, pred, taken))
> +    return;
> +  if (pred == PRED_LOOP_GUARD
> +      && edge_predicted_by_p (e, PRED_LOOP_GUARD_WITH_RECURSION,
> taken))
> +    return;
> +  /* Consider PRED_LOOP_GUARD_WITH_RECURSION superrior to
> LOOP_GUARD.
> +*/
> +  if (pred == PRED_LOOP_GUARD_WITH_RECURSION)
> +    {
> +      edge_prediction **preds = bb_predictions->get (e->src);
> +      if (preds)
> +	filter_predictions (preds, not_loop_guard_equal_edge_p, e);
> +    }
> +  predict_edge_def (e, pred, taken);
> +}
>  /* Predict edges to successors of CUR whose sources are not postdominated
> by
>     BB by PRED and recurse to all postdominators.  */
> 
> @@ -3177,10 +3206,7 @@ predict_paths_for_bb (basic_block cur,
> basic_block bb,
>  	 regions that are only reachable by abnormal edges.  We simply
>  	 prevent visiting given BB twice.  */
>        if (found)
> -	{
> -	  if (!edge_predicted_by_p (e, pred, taken))
> -            predict_edge_def (e, pred, taken);
> -	}
> +	maybe_predict_edge (e, pred, taken);
>        else if (bitmap_set_bit (visited, e->src->index))
>  	predict_paths_for_bb (e->src, e->src, pred, taken, visited, in_loop);
>      }
> @@ -3223,7 +3249,7 @@ predict_paths_leading_to_edge (edge e, enum
> br_predictor pred,
>    if (!has_nonloop_edge)
>      predict_paths_for_bb (bb, bb, pred, taken, auto_bitmap (), in_loop);
>    else
> -    predict_edge_def (e, pred, taken);
> +    maybe_predict_edge (e, pred, taken);
>  }
> 
> 
> 
>  /* This is used to carry information about basic blocks.  It is diff --git
> a/gcc/testsuite/gcc.dg/ipa/ipa-clone-2.c b/gcc/testsuite/gcc.dg/ipa/ipa-
> clone-2.c
> index d513020ee8b..53ae25a1e24 100644
> --- a/gcc/testsuite/gcc.dg/ipa/ipa-clone-2.c
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-clone-2.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-O3 -fdump-ipa-cp-details -fno-early-inlining --param ipa-
> cp-max-recursive-depth=8" } */
> +/* { dg-options "-O3 -fdump-ipa-cp-details -fno-early-inlining --param
> +ipa-cp-max-recursive-depth=8 --param=ipa-cp-eval-threshold=400" } */
> 
>  int fn();
> 

Re: Do not combine PRED_LOOP_GUARD and PRED_LOOP_GUARD_WITH_RECURSION

[Martin Liška]

On 8/12/20 1:02 PM, Tamar Christina wrote:
> Hmm the regression on exchange2 is 11%. Or 1.22% on specint 2017 overall..
> This is a rather big regression.. What would be the correct way to do this?

I can confirm that many our LNT tester configurations spotted that:

https://lnt.opensuse.org/db_default/v4/SPEC/graph?plot.0=232.407.0
https://lnt.opensuse.org/db_default/v4/SPEC/graph?plot.0=226.407.0
https://lnt.opensuse.org/db_default/v4/SPEC/graph?plot.0=294.407.0
https://lnt.opensuse.org/db_default/v4/SPEC/graph?plot.0=33.407.0

Martin