| Message ID | dac69b68-3a76-8b88-49ca-c72edbe01fca@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFA] Provide a class interface into substitute_and_fold. | expand |
On Tue, 2017-10-24 at 12:44 -0600, Jeff Law 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? > > Jeff > [...snip...] > 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 *); > +}; Should these two methods be marked OVERRIDE? (or indeed "FINAL OVERRIDE"?) This tells the human reader that they're vfuncs, and C++11 onwards can issue a warning if for some reason they stop being vfuncs (e.g. the type signature changes somewhere). (likewise for all the other overrides in subclasses of s_a_f_engine). [...snip...] Dave
On 10/24/2017 02:57 PM, David Malcolm wrote: > On Tue, 2017-10-24 at 12:44 -0600, Jeff Law 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? >> >> Jeff >> > > [...snip...] > >> 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 *); >> +}; > > Should these two methods be marked OVERRIDE? (or indeed "FINAL > OVERRIDE"?) This tells the human reader that they're vfuncs, and C++11 > onwards can issue a warning if for some reason they stop being vfuncs > (e.g. the type signature changes somewhere). > > (likewise for all the other overrides in subclasses of s_a_f_engine). OVERRIDE seems like a no-brainer. I can't offhand think of a case where we'd want to derive further. FINAL (in theory) ought to help divirt, so it seems appropriate as well. Consider that done :-) I'm also investigating if these classes need a virtual dtor. Jeff
On 10/24/2017 03:45 PM, Jeff Law wrote: > On 10/24/2017 02:57 PM, David Malcolm wrote: >> On Tue, 2017-10-24 at 12:44 -0600, Jeff Law 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? >>> >>> Jeff >>> >> >> [...snip...] >> >>> 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 *); >>> +}; >> >> Should these two methods be marked OVERRIDE? (or indeed "FINAL >> OVERRIDE"?) This tells the human reader that they're vfuncs, and C++11 >> onwards can issue a warning if for some reason they stop being vfuncs >> (e.g. the type signature changes somewhere). >> >> (likewise for all the other overrides in subclasses of s_a_f_engine). > OVERRIDE seems like a no-brainer. I can't offhand think of a case where > we'd want to derive further. FINAL (in theory) ought to help divirt, so > it seems appropriate as well. Consider that done :-) I added both and went through the usual bootstrap and regression testing. No issues (as expected). Good to get the confirmation though. > > I'm also investigating if these classes need a virtual dtor. My conclusion on the virtual dtor issue is that it's not strictly needed right now. IIUC the issue is you could do something like base *foo = new derived (); [ ... ] delete foo; If the base's destructor is not virtual and foo is a base * pointing to a derived object then the deletion invokes undefined behavior. In theory we shouldn't be doing such things :-) In fact, if there's a way to prevent this with some magic on the base class I'd love to know about it. jeff
On Tue, Oct 24, 2017 at 8:44 PM, Jeff Law <law@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) > { >
On 10/26/2017 03:24 AM, Richard Biener wrote: > On Tue, Oct 24, 2017 at 8:44 PM, Jeff Law <law@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. Good question. They're in different classes because they can and are used independently. For example, tree-complex uses the propagation engine, but not the substitution engine. EVRP uses the substitution engine, but not the propagation engine. The standard VRP algorithm uses both engines, but other than shared data (vr_values), they are independent. CCP and copy-prop are similar to VRP. Essentially one is a producer, the other a consumer. It might be possible to smash them together, but I'm not sure if that's wise or not. I do suspect that smashing them together would be easier once all the other work is done if we were to make that choice. But composition, multiple inheritance or just passing around the class instance may be better. I think that's a TBD. > > 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. Yes. The key issue is the propagation step produces vr_values and the substitution step consumes vr_values. For VRP the way I solve this is to have a vr_values class in the derived propagation engine class as well as the derived substitution engine class. When we're done with propagation we move the class instance from the propagation engine to the substitution engine. EVRP works similarly except the vr_values starts in the evrp_dom_walker class, then moves to its substitution engine. There's a bit of cleanup to do there in terms of implementation. But that's the basic model that I'm using right now. It should be fairly easy to move to a unioned class or multiple inheritance if we so desired. It shouldn't affect most of what I'm doing now around encapsulating vr_values. > > 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. Right. THey can and are used independently which is what led to having independent classes. > > 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? Right, we have decided against using multiple inheritance. So rather than using multiple inheritance I pass the vr_values object. So in my development tree I have this: class vrp_prop : public ssa_propagation_engine { public: enum ssa_prop_result visit_stmt (gimple *, edge *, tree *) FINAL OVERRIDE; enum ssa_prop_result visit_phi (gphi *) FINAL OVERRIDE; /* XXX Drop the indirection through the pointer, not needed. */ class vr_values *vr_values; }; class vrp_folder : public substitute_and_fold_engine { public: tree get_value (tree) FINAL OVERRIDE; bool fold_stmt (gimple_stmt_iterator *) FINAL OVERRIDE; class vr_values *vr_values; }; In vrp_finalize: class vrp_folder vrp_folder; vrp_folder.vr_values = vr_values; vrp_folder.substitute_and_fold (); I'm in the process of cleaning this up -- in particular there'll be a ctor in vrp_folder which will require passing in a vr_values and we'll be dropping some indirections as well. I just went through his exact cleanup yesterday with the separated evrp style range analyzer and evrp itself. > 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? Yes. I'm still encapsulating vr_values and figuring out how to deal with the various routines that want to use it. Essentially every routine has to be evaluated and either move into the classes or get passed in the vr_values class instance. There's a lot of the latter right now just to get things building so that I can see all the points where evrp and vrp are sharing code. There's also a lot of routines that want to operate on the range for a particular object. They can live outside the vr_ranges object since they're passed the relevant range. I haven't made any decisions on how I want to handle those. But it's clear that they're going to need to be shared across vrp, evrp and passes that use embedded range analysis. They could be their own class, but I suspect everything would just be a static member since they don't need a class instance. They could move into vr_values as static members. Or we could just keep them as simple C functions. > > 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. Right. I think it's the right design direction. It'll certainly require more teardown and reshuffling, but the work is, IMHO, definitely the right direction and hopefully my work will make that task easier as the goal is to be able to take the different components and trivially wire them up. I'm really focused on trying to get a clean separation of vrp and evrp. Within evrp separation of analysis from optimization (so that I can easily embed the analysis bits). At the heart of all this is encapsulation of the vr_values structure. > > 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). I'm actually hoping to post a snapshot of how this will look in a clean consumer today (sprintf bits). There's enough in place that I can do that while I continue teasing apart vrp and evrp. Jeff
On October 26, 2017 6:50:15 PM GMT+02:00, Jeff Law <law@redhat.com> wrote: >On 10/26/2017 03:24 AM, Richard Biener wrote: >> On Tue, Oct 24, 2017 at 8:44 PM, Jeff Law <law@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. >Good question. They're in different classes because they can and are >used independently. > >For example, tree-complex uses the propagation engine, but not the >substitution engine. EVRP uses the substitution engine, but not the >propagation engine. The standard VRP algorithm uses both engines, but >other than shared data (vr_values), they are independent. CCP and >copy-prop are similar to VRP. Essentially one is a producer, the other >a consumer. > >It might be possible to smash them together, but I'm not sure if that's >wise or not. I do suspect that smashing them together would be easier >once all the other work is done if we were to make that choice. But >composition, multiple inheritance or just passing around the class >instance may be better. I think that's a TBD. > > >> >> 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. >Yes. The key issue is the propagation step produces vr_values and the >substitution step consumes vr_values. > >For VRP the way I solve this is to have a vr_values class in the >derived >propagation engine class as well as the derived substitution engine >class. When we're done with propagation we move the class instance >from >the propagation engine to the substitution engine. > >EVRP works similarly except the vr_values starts in the evrp_dom_walker >class, then moves to its substitution engine. > >There's a bit of cleanup to do there in terms of implementation. But >that's the basic model that I'm using right now. It should be fairly >easy to move to a unioned class or multiple inheritance if we so >desired. It shouldn't affect most of what I'm doing now around >encapsulating vr_values. > >> >> 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. >Right. THey can and are used independently which is what led to having >independent classes. > > >> >> 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? >Right, we have decided against using multiple inheritance. So rather >than using multiple inheritance I pass the vr_values object. So in my >development tree I have this: > > >class vrp_prop : public ssa_propagation_engine >{ > public: >enum ssa_prop_result visit_stmt (gimple *, edge *, tree *) FINAL >OVERRIDE; > enum ssa_prop_result visit_phi (gphi *) FINAL OVERRIDE; > > /* XXX Drop the indirection through the pointer, not needed. */ > class vr_values *vr_values; >}; > > >class vrp_folder : public substitute_and_fold_engine >{ > public: > tree get_value (tree) FINAL OVERRIDE; > bool fold_stmt (gimple_stmt_iterator *) FINAL OVERRIDE; > class vr_values *vr_values; >}; > >In vrp_finalize: > class vrp_folder vrp_folder; > vrp_folder.vr_values = vr_values; > vrp_folder.substitute_and_fold (); > > >I'm in the process of cleaning this up -- in particular there'll be a >ctor in vrp_folder which will require passing in a vr_values and we'll >be dropping some indirections as well. > >I just went through his exact cleanup yesterday with the separated evrp >style range analyzer and evrp itself. > > >> 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? >Yes. I'm still encapsulating vr_values and figuring out how to deal >with the various routines that want to use it. Essentially every >routine has to be evaluated and either move into the classes or get >passed in the vr_values class instance. There's a lot of the latter >right now just to get things building so that I can see all the points >where evrp and vrp are sharing code. > >There's also a lot of routines that want to operate on the range for a >particular object. They can live outside the vr_ranges object since >they're passed the relevant range. I haven't made any decisions on how >I want to handle those. But it's clear that they're going to need to >be >shared across vrp, evrp and passes that use embedded range analysis. > >They could be their own class, but I suspect everything would just be a >static member since they don't need a class instance. They could move >into vr_values as static members. Or we could just keep them as simple >C functions. > > >> >> 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. >Right. I think it's the right design direction. It'll certainly >require more teardown and reshuffling, but the work is, IMHO, >definitely >the right direction and hopefully my work will make that task easier as >the goal is to be able to take the different components and trivially >wire them up. > >I'm really focused on trying to get a clean separation of vrp and evrp. >Within evrp separation of analysis from optimization (so that I can >easily embed the analysis bits). At the heart of all this is >encapsulation of the vr_values structure. > > >> >> 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). >I'm actually hoping to post a snapshot of how this will look in a clean >consumer today (sprintf bits). There's enough in place that I can do >that while I continue teasing apart vrp and evrp. Good. Nice to see we're in the same boat. Richard. >Jeff
On 10/26/2017 12:11 PM, Richard Biener wrote: > On October 26, 2017 6:50:15 PM GMT+02:00, Jeff Law <law@redhat.com> wrote: [ Big snip ] >>> 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). >> I'm actually hoping to post a snapshot of how this will look in a clean >> consumer today (sprintf bits). There's enough in place that I can do >> that while I continue teasing apart vrp and evrp. > > Good. Nice to see we're in the same boat. And just so folks can see where this is going. This is what it takes to embed a vrp-style range analyzer into the sprintf warnings pass. There's a #include to pick up the range analyzer. A new override in the sprintf dom walker and a new data member in the sprintf dom walker. You can see the calls into the range analyzer, one in the before_dom_children override and the other in the after_dom_children override to generate the context sensitive range information. There's 3 calls into a class method to get the range info. That's the general procedure for any dom walker that we'd want to have access to context sensitive range data. #include, new data member in the walker class, an enter/exit call in the {before,after}_dom_children overrides and redirecting the queries into the range analyzer rather than querying the global info. Two warts in the current implementation: First, the queries into the range analyzer occur deeper into the call chain, so we don't have trivial access to the analyzer class instance. So for now I just shove it into a global. This is a common issue I see with our codebase -- we may have some of the higher level objects in place, but when it comes to accessing those objects deeper in the call chains, we punt to globals. It's probably as much an artifact of our history as a C project as anything. Truly class-ifying the code or passing down the class instance sometimes has little/no benefit and we stop and punt to a global. That's probably not an unreasonable decision, particularly in self-contained code. But once we want to re-use the code it's a major hindrance. In fact, the whole process around cleaning up vr_data in tree-vrp is another instance of this exact issue. Anyway, I'll try to avoid ranting too much because I'm as guilty as anyone on this issue. Second. The name of the method to get the range information from the analyzer is the same as the global scoped function to get range information from the SSA_NAME. That opens us up to shadowing problems. Obviously changing the name of one or the other will need to happen. Anyway, the idea is to show the general procedure for adding range analysis to a domwalker based optimization/analysis pass. Anyway, back to cleaning up vr_values :-) Jeff commit dca6f665879a75becb08697653a47ced71a627a0 Author: Jeff Law <law@torsion.usersys.redhat.com> Date: Thu Oct 26 14:19:42 2017 -0400 Range analyzer in sprintf code diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index 7415413..4ccc140 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -80,6 +80,7 @@ along with GCC; see the file COPYING3. If not see #include "substring-locations.h" #include "diagnostic.h" #include "domwalk.h" +#include "range-analyzer.h" /* The likely worst case value of MB_LEN_MAX for the target, large enough for UTF-8. Ideally, this would be obtained by a target hook if it were @@ -121,12 +122,16 @@ class sprintf_dom_walker : public dom_walker ~sprintf_dom_walker () {} virtual edge before_dom_children (basic_block) FINAL OVERRIDE; + virtual void after_dom_children (basic_block) FINAL OVERRIDE; bool handle_gimple_call (gimple_stmt_iterator *); struct call_info; bool compute_format_length (call_info &, format_result *); + class range_analyzer range_analyzer; }; +static class vr_values *x; + class pass_sprintf_length : public gimple_opt_pass { bool fold_return_value; @@ -1143,7 +1148,7 @@ get_int_range (tree arg, HOST_WIDE_INT *pmin, HOST_WIDE_INT *pmax, { /* Try to determine the range of values of the integer argument. */ wide_int min, max; - enum value_range_type range_type = get_range_info (arg, &min, &max); + enum value_range_type range_type = x->get_range_info (arg, &min, &max); if (range_type == VR_RANGE) { HOST_WIDE_INT type_min @@ -1443,7 +1448,7 @@ format_integer (const directive &dir, tree arg) /* Try to determine the range of values of the integer argument (range information is not available for pointers). */ wide_int min, max; - enum value_range_type range_type = get_range_info (arg, &min, &max); + enum value_range_type range_type = x->get_range_info (arg, &min, &max); if (range_type == VR_RANGE) { argmin = wide_int_to_tree (argtype, min); @@ -3883,7 +3888,7 @@ sprintf_dom_walker::handle_gimple_call (gimple_stmt_iterator *gsi) of them at level 2. */ wide_int min, max; enum value_range_type range_type - = get_range_info (size, &min, &max); + = x->get_range_info (size, &min, &max); if (range_type == VR_RANGE) { dstsize @@ -3995,6 +4000,7 @@ sprintf_dom_walker::handle_gimple_call (gimple_stmt_iterator *gsi) edge sprintf_dom_walker::before_dom_children (basic_block bb) { + range_analyzer.enter (bb); for (gimple_stmt_iterator si = gsi_start_bb (bb); !gsi_end_p (si); ) { /* Iterate over statements, looking for function calls. */ @@ -4010,6 +4016,12 @@ sprintf_dom_walker::before_dom_children (basic_block bb) return NULL; } +void +sprintf_dom_walker::after_dom_children (basic_block bb) +{ + range_analyzer.leave (bb); +} + /* Execute the pass for function FUN. */ unsigned int @@ -4020,6 +4032,7 @@ pass_sprintf_length::execute (function *fun) calculate_dominance_info (CDI_DOMINATORS); sprintf_dom_walker sprintf_dom_walker; + x = &sprintf_dom_walker.range_analyzer.vr_values; sprintf_dom_walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun)); /* Clean up object size info. */
On Wed, Oct 25, 2017 at 11:20:38AM -0600, Jeff Law wrote: > On 10/24/2017 03:45 PM, Jeff Law wrote: > > On 10/24/2017 02:57 PM, David Malcolm wrote: > >> On Tue, 2017-10-24 at 12:44 -0600, Jeff Law 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? > >>> > >>> Jeff > >>> > >> > >> [...snip...] > >> > >>> 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 *); > >>> +}; > >> > >> Should these two methods be marked OVERRIDE? (or indeed "FINAL > >> OVERRIDE"?) This tells the human reader that they're vfuncs, and C++11 > >> onwards can issue a warning if for some reason they stop being vfuncs > >> (e.g. the type signature changes somewhere). > >> > >> (likewise for all the other overrides in subclasses of s_a_f_engine). > > OVERRIDE seems like a no-brainer. I can't offhand think of a case where > > we'd want to derive further. FINAL (in theory) ought to help divirt, so > > it seems appropriate as well. Consider that done :-) > I added both and went through the usual bootstrap and regression > testing. No issues (as expected). Good to get the confirmation though. > > > > > I'm also investigating if these classes need a virtual dtor. > My conclusion on the virtual dtor issue is that it's not strictly needed > right now. > > IIUC the issue is you could do something like > > base *foo = new derived (); > [ ... ] > delete foo; > > If the base's destructor is not virtual and foo is a base * pointing to > a derived object then the deletion invokes undefined behavior. > > In theory we shouldn't be doing such things :-) In fact, if there's a > way to prevent this with some magic on the base class I'd love to know > about it. -Wdelete-non-virtual-dtor (enabled by -Wall) already checks for cases that might be problematic. It seems like most of the time these classes are on the stack so you never call delete and so won't get warnings. You can also mark classes as final, if the type you call delete on can't be inherited from then it must be safe to directly call its dtor. Trev > > jeff
On 10/25/2017 06:20 PM, Jeff Law wrote: > My conclusion on the virtual dtor issue is that it's not strictly needed > right now. > > IIUC the issue is you could do something like > > base *foo = new derived (); > [ ... ] > delete foo; > > If the base's destructor is not virtual and foo is a base * pointing to > a derived object then the deletion invokes undefined behavior. > > In theory we shouldn't be doing such things :-) In fact, if there's a > way to prevent this with some magic on the base class I'd love to know > about it. There is: make the base class destructor protected. Thanks, Pedro Alves
On 10/30/2017 03:32 AM, Pedro Alves wrote: > On 10/25/2017 06:20 PM, Jeff Law wrote: > >> My conclusion on the virtual dtor issue is that it's not strictly needed >> right now. >> >> IIUC the issue is you could do something like >> >> base *foo = new derived (); >> [ ... ] >> delete foo; >> >> If the base's destructor is not virtual and foo is a base * pointing to >> a derived object then the deletion invokes undefined behavior. >> >> In theory we shouldn't be doing such things :-) In fact, if there's a >> way to prevent this with some magic on the base class I'd love to know >> about it. > > There is: make the base class destructor protected. I was just reviewing our coding conventions, mostly because I thought we had something about using protected. While doing so I see that our conventions explicitly indicate that a polymorphic class should have a virtual destructor. So even though there's another solution and I don't think these objects are likely to suffer from the problems noted above, I'm just going to change things to follow our existing conventions -- just because I'm not dynamically allocating these objects doesn't mean someone else won't later.. Jeff
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) {