On Tue, Oct 24, 2017 at 8:44 PM, Jeff Law <l...@redhat.com> wrote:
> This is similar to the introduction of the ssa_propagate_engine, but for
> the substitution/replacements bits.
>
> In a couple places the pass specific virtual functions are just wrappers
> around existing functions. A good example of this is
> ccp_folder::get_value. Many other routines in tree-ssa-ccp.c want to
> use get_constant_value. Some may be convertable to use the class
> instance, but I haven't looked closely.
>
> Another example is vrp_folder::get_value. In this case we're wrapping
> op_with_constant_singleton_value. In a later patch that moves into the
> to-be-introduced vr_values class so we'll delegate to that class rather
> than wrap.
>
> FWIW I did look at having a single class for the propagation engine and
> the substitution engine. That turned out to be a bit problematical due
> to the calls into the substitution engine from the evrp bits which don't
> use the propagation engine at all. Given propagation and substitution
> are distinct concepts I ultimately decided the cleanest path forward was
> to keep the two classes separate.
>
> Bootstrapped and regression tested on x86_64. OK for the trunk?
So what I don't understand in this 2 part series is why you put
substitute-and-fold into a different class.
This makes it difficult for users to inherit and put the lattice in
the deriving class as we have the visit routines which will update
the lattice and the get_value hook which queries it.
So from maintaining the state for the users using a single
class whould be more appropriate. Of course it seems like
substitute-and-fold can be used without using the SSA
propagator itself and the SSA propagator can be used
without the substitute and fold engine.
IIRC we decided against using multiple inheritance? Which
means a user would put the lattice in the SSA propagation
engine derived class and do the inheriting via composition
as member in the substitute_and_fold engine?
Your patches keep things simple (aka the lattice and most
functions are globals), but is composition what you had
in mind when doing this class-ification?
Just thinking that if we can encapsulate the propagation
part of all our propagators we should be able to make
them work on ranges and instantiated by other consumers
(basically what you want to do for EVRP), like a hypothetical
static analysis pass.
Both patches look ok to me though it would be nice to
do the actual composition with a comment that the
lattices might be moved here (if all workers became
member functions as well).
Thanks,
Richard.
> Jeff
>
>
>
> * tree-ssa-ccp.c (ccp_folder): New class derived from
> substitute_and_fold_engine.
> (ccp_folder::get_value): New member function.
> (ccp_folder::fold_stmt): Renamed from ccp_fold_stmt.
> (ccp_fold_stmt): Remove prototype.
> (ccp_finalize): Call substitute_and_fold from the ccp_class.
> * tree-ssa-copy.c (copy_folder): New class derived from
> substitute_and_fold_engine.
> (copy_folder::get_value): Renamed from get_value.
> (fini_copy_prop): Call substitute_and_fold from copy_folder class.
> * tree-vrp.c (vrp_folder): New class derived from
> substitute_and_fold_engine.
> (vrp_folder::fold_stmt): Renamed from vrp_fold_stmt.
> (vrp_folder::get_value): New member function.
> (vrp_finalize): Call substitute_and_fold from vrp_folder class.
> (evrp_dom_walker::before_dom_children): Similarly for replace_uses_in.
> * tree-ssa-propagate.h (substitute_and_fold_engine): New class to
> provide a class interface to folder/substitute routines.
> (ssa_prop_fold_stmt_fn): Remove typedef.
> (ssa_prop_get_value_fn): Likewise.
> (subsitute_and_fold): Remove prototype.
> (replace_uses_in): Likewise.
> * tree-ssa-propagate.c (substitute_and_fold_engine::replace_uses_in):
> Renamed from replace_uses_in. Call the virtual member function
> (substitute_and_fold_engine::replace_phi_args_in): Similarly.
> (substitute_and_fold_dom_walker): Remove initialization of
> data member entries for calbacks. Add substitute_and_fold_engine
> member and initialize it.
> (substitute_and_fold_dom_walker::before_dom_children0: Use the
> member functions for get_value, replace_phi_args_in c
> replace_uses_in, and fold_stmt calls.
> (substitute_and_fold_engine::substitute_and_fold): Renamed from
> substitute_and_fold. Remove assert. Update ctor call.
>
>
> diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
> index fec562e..da06172 100644
> --- a/gcc/tree-ssa-ccp.c
> +++ b/gcc/tree-ssa-ccp.c
> @@ -188,7 +188,6 @@ static ccp_prop_value_t *const_val;
> static unsigned n_const_val;
>
> static void canonicalize_value (ccp_prop_value_t *);
> -static bool ccp_fold_stmt (gimple_stmt_iterator *);
> static void ccp_lattice_meet (ccp_prop_value_t *, ccp_prop_value_t *);
>
> /* Dump constant propagation value VAL to file OUTF prefixed by PREFIX. */
> @@ -909,6 +908,24 @@ do_dbg_cnt (void)
> }
>
>
> +/* We want to provide our own GET_VALUE and FOLD_STMT virtual methods. */
> +class ccp_folder : public substitute_and_fold_engine
> +{
> + public:
> + tree get_value (tree);
> + bool fold_stmt (gimple_stmt_iterator *);
> +};
> +
> +/* This method just wraps GET_CONSTANT_VALUE for now. Over time
> + naked calls to GET_CONSTANT_VALUE should be eliminated in favor
> + of calling member functions. */
> +
> +tree
> +ccp_folder::get_value (tree op)
> +{
> + return get_constant_value (op);
> +}
> +
> /* Do final substitution of propagated values, cleanup the flowgraph and
> free allocated storage. If NONZERO_P, record nonzero bits.
>
> @@ -967,7 +984,8 @@ ccp_finalize (bool nonzero_p)
> }
>
> /* Perform substitutions based on the known constant values. */
> - something_changed = substitute_and_fold (get_constant_value,
> ccp_fold_stmt);
> + class ccp_folder ccp_folder;
> + something_changed = ccp_folder.substitute_and_fold ();
>
> free (const_val);
> const_val = NULL;
> @@ -2176,8 +2194,8 @@ fold_builtin_alloca_with_align (gimple *stmt)
> /* Fold the stmt at *GSI with CCP specific information that propagating
> and regular folding does not catch. */
>
> -static bool
> -ccp_fold_stmt (gimple_stmt_iterator *gsi)
> +bool
> +ccp_folder::fold_stmt (gimple_stmt_iterator *gsi)
> {
> gimple *stmt = gsi_stmt (*gsi);
>
> diff --git a/gcc/tree-ssa-copy.c b/gcc/tree-ssa-copy.c
> index a57535c..e45d188 100644
> --- a/gcc/tree-ssa-copy.c
> +++ b/gcc/tree-ssa-copy.c
> @@ -489,10 +489,16 @@ init_copy_prop (void)
> }
> }
>
> +class copy_folder : public substitute_and_fold_engine
> +{
> + public:
> + tree get_value (tree);
> +};
> +
> /* Callback for substitute_and_fold to get at the final copy-of values. */
>
> -static tree
> -get_value (tree name)
> +tree
> +copy_folder::get_value (tree name)
> {
> tree val;
> if (SSA_NAME_VERSION (name) >= n_copy_of)
> @@ -557,7 +563,8 @@ fini_copy_prop (void)
> }
> }
>
> - bool changed = substitute_and_fold (get_value, NULL);
> + class copy_folder copy_folder;
> + bool changed = copy_folder.substitute_and_fold ();
> if (changed)
> {
> free_numbers_of_iterations_estimates (cfun);
> diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
> index 90df285..62955be 100644
> --- a/gcc/tree-ssa-propagate.c
> +++ b/gcc/tree-ssa-propagate.c
> @@ -853,7 +853,7 @@ static struct prop_stats_d prop_stats;
> PROP_VALUE. Return true if at least one reference was replaced. */
>
> bool
> -replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value)
> +substitute_and_fold_engine::replace_uses_in (gimple *stmt)
> {
> bool replaced = false;
> use_operand_p use;
> @@ -862,7 +862,7 @@ replace_uses_in (gimple *stmt, ssa_prop_get_value_fn
> get_value)
> FOR_EACH_SSA_USE_OPERAND (use, stmt, iter, SSA_OP_USE)
> {
> tree tuse = USE_FROM_PTR (use);
> - tree val = (*get_value) (tuse);
> + tree val = get_value (tuse);
>
> if (val == tuse || val == NULL_TREE)
> continue;
> @@ -891,8 +891,8 @@ replace_uses_in (gimple *stmt, ssa_prop_get_value_fn
> get_value)
> /* Replace propagated values into all the arguments for PHI using the
> values from PROP_VALUE. */
>
> -static bool
> -replace_phi_args_in (gphi *phi, ssa_prop_get_value_fn get_value)
> +bool
> +substitute_and_fold_engine::replace_phi_args_in (gphi *phi)
> {
> size_t i;
> bool replaced = false;
> @@ -909,7 +909,7 @@ replace_phi_args_in (gphi *phi, ssa_prop_get_value_fn
> get_value)
>
> if (TREE_CODE (arg) == SSA_NAME)
> {
> - tree val = (*get_value) (arg);
> + tree val = get_value (arg);
>
> if (val && val != arg && may_propagate_copy (arg, val))
> {
> @@ -960,10 +960,10 @@ class substitute_and_fold_dom_walker : public dom_walker
> {
> public:
> substitute_and_fold_dom_walker (cdi_direction direction,
> - ssa_prop_get_value_fn get_value_fn_,
> - ssa_prop_fold_stmt_fn fold_fn_)
> - : dom_walker (direction), get_value_fn (get_value_fn_),
> - fold_fn (fold_fn_), something_changed (false)
> + class substitute_and_fold_engine *engine)
> + : dom_walker (direction),
> + something_changed (false),
> + substitute_and_fold_engine (engine)
> {
> stmts_to_remove.create (0);
> stmts_to_fixup.create (0);
> @@ -979,12 +979,12 @@ public:
> virtual edge before_dom_children (basic_block);
> virtual void after_dom_children (basic_block) {}
>
> - ssa_prop_get_value_fn get_value_fn;
> - ssa_prop_fold_stmt_fn fold_fn;
> bool something_changed;
> vec<gimple *> stmts_to_remove;
> vec<gimple *> stmts_to_fixup;
> bitmap need_eh_cleanup;
> +
> + class substitute_and_fold_engine *substitute_and_fold_engine;
> };
>
> edge
> @@ -1001,7 +1001,7 @@ substitute_and_fold_dom_walker::before_dom_children
> (basic_block bb)
> continue;
> if (res && TREE_CODE (res) == SSA_NAME)
> {
> - tree sprime = get_value_fn (res);
> + tree sprime = substitute_and_fold_engine->get_value (res);
> if (sprime
> && sprime != res
> && may_propagate_copy (res, sprime))
> @@ -1010,7 +1010,7 @@ substitute_and_fold_dom_walker::before_dom_children
> (basic_block bb)
> continue;
> }
> }
> - something_changed |= replace_phi_args_in (phi, get_value_fn);
> + something_changed |= substitute_and_fold_engine->replace_phi_args_in
> (phi);
> }
>
> /* Propagate known values into stmts. In some case it exposes
> @@ -1027,7 +1027,7 @@ substitute_and_fold_dom_walker::before_dom_children
> (basic_block bb)
> tree lhs = gimple_get_lhs (stmt);
> if (lhs && TREE_CODE (lhs) == SSA_NAME)
> {
> - tree sprime = get_value_fn (lhs);
> + tree sprime = substitute_and_fold_engine->get_value (lhs);
> if (sprime
> && sprime != lhs
> && may_propagate_copy (lhs, sprime)
> @@ -1056,7 +1056,7 @@ substitute_and_fold_dom_walker::before_dom_children
> (basic_block bb)
> && gimple_call_noreturn_p (stmt));
>
> /* Replace real uses in the statement. */
> - did_replace |= replace_uses_in (stmt, get_value_fn);
> + did_replace |= substitute_and_fold_engine->replace_uses_in (stmt);
>
> /* If we made a replacement, fold the statement. */
> if (did_replace)
> @@ -1069,16 +1069,13 @@ substitute_and_fold_dom_walker::before_dom_children
> (basic_block bb)
> /* Some statements may be simplified using propagator
> specific information. Do this before propagating
> into the stmt to not disturb pass specific information. */
> - if (fold_fn)
> + update_stmt_if_modified (stmt);
> + if (substitute_and_fold_engine->fold_stmt(&i))
> {
> - update_stmt_if_modified (stmt);
> - if ((*fold_fn)(&i))
> - {
> - did_replace = true;
> - prop_stats.num_stmts_folded++;
> - stmt = gsi_stmt (i);
> - gimple_set_modified (stmt, true);
> - }
> + did_replace = true;
> + prop_stats.num_stmts_folded++;
> + stmt = gsi_stmt (i);
> + gimple_set_modified (stmt, true);
> }
>
> /* If this is a control statement the propagator left edges
> @@ -1164,19 +1161,15 @@ substitute_and_fold_dom_walker::before_dom_children
> (basic_block bb)
> Return TRUE when something changed. */
>
> bool
> -substitute_and_fold (ssa_prop_get_value_fn get_value_fn,
> - ssa_prop_fold_stmt_fn fold_fn)
> +substitute_and_fold_engine::substitute_and_fold (void)
> {
> - gcc_assert (get_value_fn);
> -
> if (dump_file && (dump_flags & TDF_DETAILS))
> fprintf (dump_file, "\nSubstituting values and folding statements\n\n");
>
> memset (&prop_stats, 0, sizeof (prop_stats));
>
> calculate_dominance_info (CDI_DOMINATORS);
> - substitute_and_fold_dom_walker walker(CDI_DOMINATORS,
> - get_value_fn, fold_fn);
> + substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
> walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>
> /* We cannot remove stmts during the BB walk, especially not release
> diff --git a/gcc/tree-ssa-propagate.h b/gcc/tree-ssa-propagate.h
> index cff9e53..629ae77 100644
> --- a/gcc/tree-ssa-propagate.h
> +++ b/gcc/tree-ssa-propagate.h
> @@ -61,17 +61,11 @@ enum ssa_prop_result {
> };
>
>
> -/* Call-back functions used by the value propagation engine. */
> -typedef bool (*ssa_prop_fold_stmt_fn) (gimple_stmt_iterator *gsi);
> -typedef tree (*ssa_prop_get_value_fn) (tree);
> -
> -
> extern bool valid_gimple_rhs_p (tree);
> extern void move_ssa_defining_stmt_for_defs (gimple *, gimple *);
> extern bool update_gimple_call (gimple_stmt_iterator *, tree, int, ...);
> extern bool update_call_from_tree (gimple_stmt_iterator *, tree);
> extern bool stmt_makes_single_store (gimple *);
> -extern bool substitute_and_fold (ssa_prop_get_value_fn,
> ssa_prop_fold_stmt_fn);
> extern bool may_propagate_copy (tree, tree);
> extern bool may_propagate_copy_into_stmt (gimple *, tree);
> extern bool may_propagate_copy_into_asm (tree);
> @@ -79,7 +73,6 @@ extern void propagate_value (use_operand_p, tree);
> extern void replace_exp (use_operand_p, tree);
> extern void propagate_tree_value (tree *, tree);
> extern void propagate_tree_value_into_stmt (gimple_stmt_iterator *, tree);
> -extern bool replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value);
>
> /* Public interface into the SSA propagation engine. Clients should inherit
> from this class and provide their own visitors. */
> @@ -104,4 +97,14 @@ class ssa_propagation_engine
>
> };
>
> +class substitute_and_fold_engine
> +{
> + public:
> + bool substitute_and_fold (void);
> + bool replace_uses_in (gimple *);
> + virtual bool fold_stmt (gimple_stmt_iterator *) { return false; }
> + virtual tree get_value (tree) { return NULL_TREE; }
> + bool replace_phi_args_in (gphi *);
> +};
> +
> #endif /* _TREE_SSA_PROPAGATE_H */
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index e8fce3e..ea8ceee 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -10536,10 +10536,17 @@ fold_predicate_in (gimple_stmt_iterator *si)
> return false;
> }
>
> +class vrp_folder : public substitute_and_fold_engine
> +{
> + public:
> + tree get_value (tree);
> + bool fold_stmt (gimple_stmt_iterator *);
> +};
> +
> /* Callback for substitute_and_fold folding the stmt at *SI. */
>
> -static bool
> -vrp_fold_stmt (gimple_stmt_iterator *si)
> +bool
> +vrp_folder::fold_stmt (gimple_stmt_iterator *si)
> {
> if (fold_predicate_in (si))
> return true;
> @@ -10547,6 +10554,18 @@ vrp_fold_stmt (gimple_stmt_iterator *si)
> return simplify_stmt_using_ranges (si);
> }
>
> +/* If OP has a value range with a single constant value return that,
> + otherwise return NULL_TREE. This returns OP itself if OP is a
> + constant.
> +
> + Implemented as a pure wrapper right now, but this will change. */
> +
> +tree
> +vrp_folder::get_value (tree op)
> +{
> + return op_with_constant_singleton_value_range (op);
> +}
> +
> /* Return the LHS of any ASSERT_EXPR where OP appears as the first
> argument to the ASSERT_EXPR and in which the ASSERT_EXPR dominates
> BB. If no such ASSERT_EXPR is found, return OP. */
> @@ -10888,7 +10907,8 @@ vrp_finalize (bool warn_array_bounds_p)
> wi::to_wide (vr_value[i]->max));
> }
>
> - substitute_and_fold (op_with_constant_singleton_value_range,
> vrp_fold_stmt);
> + class vrp_folder vrp_folder;
> + vrp_folder.substitute_and_fold ();
>
> if (warn_array_bounds && warn_array_bounds_p)
> check_all_array_refs ();
> @@ -11225,8 +11245,8 @@ evrp_dom_walker::before_dom_children (basic_block bb)
> }
>
> /* Try folding stmts with the VR discovered. */
> - bool did_replace
> - = replace_uses_in (stmt, op_with_constant_singleton_value_range);
> + class vrp_folder vrp_folder;
> + bool did_replace = vrp_folder.replace_uses_in (stmt);
> if (fold_stmt (&gsi, follow_single_use_edges)
> || did_replace)
> {
>
