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
