RE: [PATCH] tree-optimization/114589 - remove profile based sink heuristics

[Richard Biener <rguenther@suse.de>]

On Fri, 14 Jun 2024, Li, Pan2 wrote:

> Hi Richard,
> 
> Here is one PR related to this patch (by git bisect), details as below.
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115458
> 
> I am still trying to narrow down which change caused this failure or any hints here?

It definitely looks like a latent issue being triggered.  Either in LRA
or in how the target presents itself.

Richard.

> Pan
> 
> -----Original Message-----
> From: Richard Biener <rguenther@suse.de> 
> Sent: Wednesday, May 15, 2024 5:39 PM
> To: gcc-patches@gcc.gnu.org
> Subject: [PATCH] tree-optimization/114589 - remove profile based sink heuristics
> 
> The following removes the profile based heuristic limiting sinking
> and instead uses post-dominators to avoid sinking to places that
> are executed under the same conditions as the earlier location which
> the profile based heuristic should have guaranteed as well.
> 
> To avoid regressing this moves the empty-latch check to cover all
> sink cases.
> 
> It also stream-lines the resulting select_best_block a bit but avoids
> adjusting heuristics more with this change.  gfortran.dg/streamio_9.f90
> starts execute failing with this on x86_64 with -m32 because the
> (float)i * 9.9999...e-7 compute is sunk across a STOP causing it
> to be no longer spilled and thus the compare failing due to excess
> precision.  The patch adds -ffloat-store to avoid this, following
> other similar testcases.
> 
> This change doesn't fix the testcase in the PR on itself.
> 
> Bootstrapped on x86_64-unknown-linux-gnu, re-testing in progress.
> 
> 	PR tree-optimization/114589
> 	* tree-ssa-sink.cc (select_best_block): Remove profile-based
> 	heuristics.  Instead reject sink locations that sink
>         to post-dominators.  Move empty latch check here from
> 	statement_sink_location.  Also consider early_bb for the
> 	loop depth check.
> 	(statement_sink_location): Remove superfluous check.  Remove
> 	empty latch check.
> 	(pass_sink_code::execute): Compute/release post-dominators.
> 
> 	* gfortran.dg/streamio_9.f90: Use -ffloat-store to avoid
> 	excess precision when not spilling.
> ---
>  gcc/testsuite/gfortran.dg/streamio_9.f90 |  1 +
>  gcc/tree-ssa-sink.cc                     | 62 ++++++++----------------
>  2 files changed, 20 insertions(+), 43 deletions(-)
> 
> diff --git a/gcc/testsuite/gfortran.dg/streamio_9.f90 b/gcc/testsuite/gfortran.dg/streamio_9.f90
> index b6bddb973f8..f29ded6ba54 100644
> --- a/gcc/testsuite/gfortran.dg/streamio_9.f90
> +++ b/gcc/testsuite/gfortran.dg/streamio_9.f90
> @@ -1,4 +1,5 @@
>  ! { dg-do run }
> +! { dg-options "-ffloat-store" }
>  ! PR29053 Stream IO test 9.
>  ! Contributed by Jerry DeLisle <jvdelisle@gcc.gnu.org>.
>  ! Test case derived from that given in PR by Steve Kargl.
> diff --git a/gcc/tree-ssa-sink.cc b/gcc/tree-ssa-sink.cc
> index 2f90acb7ef4..2188b7523c7 100644
> --- a/gcc/tree-ssa-sink.cc
> +++ b/gcc/tree-ssa-sink.cc
> @@ -178,15 +178,7 @@ nearest_common_dominator_of_uses (def_operand_p def_p, bool *debug_stmts)
>  
>     We want the most control dependent block in the shallowest loop nest.
>  
> -   If the resulting block is in a shallower loop nest, then use it.  Else
> -   only use the resulting block if it has significantly lower execution
> -   frequency than EARLY_BB to avoid gratuitous statement movement.  We
> -   consider statements with VOPS more desirable to move.
> -
> -   This pass would obviously benefit from PDO as it utilizes block
> -   frequencies.  It would also benefit from recomputing frequencies
> -   if profile data is not available since frequencies often get out
> -   of sync with reality.  */
> +   If the resulting block is in a shallower loop nest, then use it.  */
>  
>  static basic_block
>  select_best_block (basic_block early_bb,
> @@ -195,18 +187,17 @@ select_best_block (basic_block early_bb,
>  {
>    basic_block best_bb = late_bb;
>    basic_block temp_bb = late_bb;
> -  int threshold;
>  
>    while (temp_bb != early_bb)
>      {
> +      /* Walk up the dominator tree, hopefully we'll find a shallower
> +	 loop nest.  */
> +      temp_bb = get_immediate_dominator (CDI_DOMINATORS, temp_bb);
> +
>        /* If we've moved into a lower loop nest, then that becomes
>  	 our best block.  */
>        if (bb_loop_depth (temp_bb) < bb_loop_depth (best_bb))
>  	best_bb = temp_bb;
> -
> -      /* Walk up the dominator tree, hopefully we'll find a shallower
> - 	 loop nest.  */
> -      temp_bb = get_immediate_dominator (CDI_DOMINATORS, temp_bb);
>      }
>  
>    /* Placing a statement before a setjmp-like function would be invalid
> @@ -221,6 +212,16 @@ select_best_block (basic_block early_bb,
>    if (bb_loop_depth (best_bb) < bb_loop_depth (early_bb))
>      return best_bb;
>  
> +  /* Do not move stmts to post-dominating places on the same loop depth.  */
> +  if (dominated_by_p (CDI_POST_DOMINATORS, early_bb, best_bb))
> +    return early_bb;
> +
> +  /* If the latch block is empty, don't make it non-empty by sinking
> +     something into it.  */
> +  if (best_bb == early_bb->loop_father->latch
> +      && empty_block_p (best_bb))
> +    return early_bb;
> +
>    /* Avoid turning an unconditional read into a conditional one when we
>       still might want to perform vectorization.  */
>    if (best_bb->loop_father == early_bb->loop_father
> @@ -233,28 +234,7 @@ select_best_block (basic_block early_bb,
>        && !dominated_by_p (CDI_DOMINATORS, best_bb->loop_father->latch, best_bb))
>      return early_bb;
>  
> -  /* Get the sinking threshold.  If the statement to be moved has memory
> -     operands, then increase the threshold by 7% as those are even more
> -     profitable to avoid, clamping at 100%.  */
> -  threshold = param_sink_frequency_threshold;
> -  if (gimple_vuse (stmt) || gimple_vdef (stmt))
> -    {
> -      threshold += 7;
> -      if (threshold > 100)
> -	threshold = 100;
> -    }
> -
> -  /* If BEST_BB is at the same nesting level, then require it to have
> -     significantly lower execution frequency to avoid gratuitous movement.  */
> -  if (bb_loop_depth (best_bb) == bb_loop_depth (early_bb)
> -      /* If result of comparsion is unknown, prefer EARLY_BB.
> -	 Thus use !(...>=..) rather than (...<...)  */
> -      && !(best_bb->count * 100 >= early_bb->count * threshold))
> -    return best_bb;
> -
> -  /* No better block found, so return EARLY_BB, which happens to be the
> -     statement's original block.  */
> -  return early_bb;
> +  return best_bb;
>  }
>  
>  /* Given a statement (STMT) and the basic block it is currently in (FROMBB),
> @@ -452,13 +432,7 @@ statement_sink_location (gimple *stmt, basic_block frombb,
>      return false;
>    
>    sinkbb = select_best_block (frombb, sinkbb, stmt);
> -  if (!sinkbb || sinkbb == frombb)
> -    return false;
> -
> -  /* If the latch block is empty, don't make it non-empty by sinking
> -     something into it.  */
> -  if (sinkbb == frombb->loop_father->latch
> -      && empty_block_p (sinkbb))
> +  if (sinkbb == frombb)
>      return false;
>  
>    *togsi = gsi_after_labels (sinkbb);
> @@ -822,6 +796,7 @@ pass_sink_code::execute (function *fun)
>    mark_dfs_back_edges (fun);
>    memset (&sink_stats, 0, sizeof (sink_stats));
>    calculate_dominance_info (CDI_DOMINATORS);
> +  calculate_dominance_info (CDI_POST_DOMINATORS);
>  
>    virtual_operand_live vop_live;
>  
> @@ -833,6 +808,7 @@ pass_sink_code::execute (function *fun)
>  
>    statistics_counter_event (fun, "Sunk statements", sink_stats.sunk);
>    statistics_counter_event (fun, "Commoned stores", sink_stats.commoned);
> +  free_dominance_info (CDI_POST_DOMINATORS);
>    remove_fake_exit_edges ();
>    loop_optimizer_finalize ();
>  
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)