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
