[RFA][PATCH] Provide a class interface into substitute_and_fold.

[Jeff Law]

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