gimple ssa: Put SCCOPY logic into a class

  Hello everybody,

In pr113054[1] Andrew said that he doesn't like the 'dead_stmts' static
variable I used when implementing the sccopy pass.  We agreed that wrapping
the relevant code from the pass in a class would be most likely the best
solution.  Here is a patch that does exactly that.  I waited until stage 1 to
submit it.

Bootstrapped and regtested on x86_64.  Is the patch ok to be pushed to trunk?

Cheers,
Filip Kastl


[1]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113054


-- 8< --


Currently the main logic of the sccopy pass is implemented as static
functions.  This patch instead puts the code into a class.  This also
gets rid of a global variable (dead_stmts).

gcc/ChangeLog:

	* gimple-ssa-sccopy.cc (class scc_copy_prop): New class.
	(replace_scc_by_value): Put into...
	(scc_copy_prop::replace_scc_by_value): ...scc_copy_prop.
	(sccopy_visit_op): Put into...
	(scc_copy_prop::visit_op): ...scc_copy_prop.
	(sccopy_propagate): Put into...
	(scc_copy_prop::propagate): ...scc_copy_prop.
	(init_sccopy): Replace by...
	(scc_copy_prop::scc_copy_prop): ...the construtor.
	(finalize_sccopy): Replace by...
	(scc_copy_prop::~scc_copy_prop): ...the destructor.
	(pass_sccopy::execute): Use scc_copy_prop.

Signed-off-by: Filip Kastl <fkastl@suse.cz>
---
 gcc/gimple-ssa-sccopy.cc | 66 ++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 29 deletions(-)
  

On Tue, 6 Aug 2024, Filip Kastl wrote:

> Hello everybody,
> 
> In pr113054[1] Andrew said that he doesn't like the 'dead_stmts' static
> variable I used when implementing the sccopy pass.  We agreed that wrapping
> the relevant code from the pass in a class would be most likely the best
> solution.  Here is a patch that does exactly that.  I waited until stage 1 to
> submit it.
> 
> Bootstrapped and regtested on x86_64.  Is the patch ok to be pushed to trunk?

OK.

Richard.

> Cheers,
> Filip Kastl
> 
> 
> [1]
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113054
> 
> 
> -- 8< --
> 
> 
> Currently the main logic of the sccopy pass is implemented as static
> functions.  This patch instead puts the code into a class.  This also
> gets rid of a global variable (dead_stmts).
> 
> gcc/ChangeLog:
> 
> 	* gimple-ssa-sccopy.cc (class scc_copy_prop): New class.
> 	(replace_scc_by_value): Put into...
> 	(scc_copy_prop::replace_scc_by_value): ...scc_copy_prop.
> 	(sccopy_visit_op): Put into...
> 	(scc_copy_prop::visit_op): ...scc_copy_prop.
> 	(sccopy_propagate): Put into...
> 	(scc_copy_prop::propagate): ...scc_copy_prop.
> 	(init_sccopy): Replace by...
> 	(scc_copy_prop::scc_copy_prop): ...the construtor.
> 	(finalize_sccopy): Replace by...
> 	(scc_copy_prop::~scc_copy_prop): ...the destructor.
> 	(pass_sccopy::execute): Use scc_copy_prop.
> 
> Signed-off-by: Filip Kastl <fkastl@suse.cz>
> ---
>  gcc/gimple-ssa-sccopy.cc | 66 ++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/gcc/gimple-ssa-sccopy.cc b/gcc/gimple-ssa-sccopy.cc
> index 191a4c0b451..d9eaeab4abb 100644
> --- a/gcc/gimple-ssa-sccopy.cc
> +++ b/gcc/gimple-ssa-sccopy.cc
> @@ -94,11 +94,6 @@ along with GCC; see the file COPYING3.  If not see
>  
>  namespace {
>  
> -/* Bitmap tracking statements which were propagated to be removed at the end of
> -   the pass.  */
> -
> -static bitmap dead_stmts;
> -
>  /* State of vertex during SCC discovery.
>  
>     unvisited  Vertex hasn't yet been popped from worklist.
> @@ -459,11 +454,33 @@ get_all_stmt_may_generate_copy (void)
>    return result;
>  }
>  
> +/* SCC copy propagation
> +
> +   'scc_copy_prop::propagate ()' is the main function of this pass.  */
> +
> +class scc_copy_prop
> +{
> +public:
> +  scc_copy_prop ();
> +  ~scc_copy_prop ();
> +  void propagate ();
> +
> +private:
> +  /* Bitmap tracking statements which were propagated so that they can be
> +     removed at the end of the pass.  */
> +  bitmap dead_stmts;
> +
> +  void visit_op (tree op, hash_set<tree> &outer_ops,
> +				hash_set<gimple *> &scc_set, bool &is_inner,
> +				tree &last_outer_op);
> +  void replace_scc_by_value (vec<gimple *> scc, tree val);
> +};
> +
>  /* For each statement from given SCC, replace its usages by value
>     VAL.  */
>  
> -static void
> -replace_scc_by_value (vec<gimple *> scc, tree val)
> +void
> +scc_copy_prop::replace_scc_by_value (vec<gimple *> scc, tree val)
>  {
>    for (gimple *stmt : scc)
>      {
> @@ -476,12 +493,12 @@ replace_scc_by_value (vec<gimple *> scc, tree val)
>      fprintf (dump_file, "Replacing SCC of size %d\n", scc.length ());
>  }
>  
> -/* Part of 'sccopy_propagate ()'.  */
> +/* Part of 'scc_copy_prop::propagate ()'.  */
>  
> -static void
> -sccopy_visit_op (tree op, hash_set<tree> &outer_ops,
> -		 hash_set<gimple *> &scc_set, bool &is_inner,
> -		 tree &last_outer_op)
> +void
> +scc_copy_prop::visit_op (tree op, hash_set<tree> &outer_ops,
> +			 hash_set<gimple *> &scc_set, bool &is_inner,
> +			 tree &last_outer_op)
>  {
>    bool op_in_scc = false;
>  
> @@ -539,8 +556,8 @@ sccopy_visit_op (tree op, hash_set<tree> &outer_ops,
>       Braun, Buchwald, Hack, Leissa, Mallon, Zwinkau, 2013, LNCS vol. 7791,
>       Section 3.2.  */
>  
> -static void
> -sccopy_propagate ()
> +void
> +scc_copy_prop::propagate ()
>  {
>    auto_vec<gimple *> useful_stmts = get_all_stmt_may_generate_copy ();
>    scc_discovery discovery;
> @@ -575,14 +592,12 @@ sccopy_propagate ()
>  		for (j = 0; j < gimple_phi_num_args (phi); j++)
>  		  {
>  		    op = gimple_phi_arg_def (phi, j);
> -		    sccopy_visit_op (op, outer_ops, scc_set, is_inner,
> -				   last_outer_op);
> +		    visit_op (op, outer_ops, scc_set, is_inner, last_outer_op);
>  		  }
>  		break;
>  	      case GIMPLE_ASSIGN:
>  		op = gimple_assign_rhs1 (stmt);
> -		sccopy_visit_op (op, outer_ops, scc_set, is_inner,
> -			       last_outer_op);
> +		visit_op (op, outer_ops, scc_set, is_inner, last_outer_op);
>  		break;
>  	      default:
>  		gcc_unreachable ();
> @@ -613,19 +628,13 @@ sccopy_propagate ()
>      }
>  }
>  
> -/* Called when pass execution starts.  */
> -
> -static void
> -init_sccopy (void)
> +scc_copy_prop::scc_copy_prop ()
>  {
>    /* For propagated statements.  */
>    dead_stmts = BITMAP_ALLOC (NULL);
>  }
>  
> -/* Called before pass execution ends.  */
> -
> -static void
> -finalize_sccopy (void)
> +scc_copy_prop::~scc_copy_prop ()
>  {
>    /* Remove all propagated statements.  */
>    simple_dce_from_worklist (dead_stmts);
> @@ -668,9 +677,8 @@ public:
>  unsigned
>  pass_sccopy::execute (function *)
>  {
> -  init_sccopy ();
> -  sccopy_propagate ();
> -  finalize_sccopy ();
> +  scc_copy_prop sccopy;
> +  sccopy.propagate ();
>    return 0;
>  }
>  
>
  

On Tue 2024-08-06 15:14:32, Richard Biener wrote:
> On Tue, 6 Aug 2024, Filip Kastl wrote:
> 
> > Hello everybody,
> > 
> > In pr113054[1] Andrew said that he doesn't like the 'dead_stmts' static
> > variable I used when implementing the sccopy pass.  We agreed that wrapping
> > the relevant code from the pass in a class would be most likely the best
> > solution.  Here is a patch that does exactly that.  I waited until stage 1 to
> > submit it.
> > 
> > Bootstrapped and regtested on x86_64.  Is the patch ok to be pushed to trunk?
> 
> OK.
> 
> Richard.
> 

Thanks, pushed.

Filip