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