Re: Staging in vrp cleanups

[Jeff Law <law at redhat dot com>]

On 11/07/2017 02:52 AM, Richard Biener wrote:
> On Mon, Nov 6, 2017 at 6:01 PM, Jeff Law <law@redhat.com> wrote:
>>
>>
>> So I spent a fair amount of time over the weekend trying to figure out
>> how to stage in the vrp cleanups.  I don't want to drop a massive
>> unreviewable kit on everyone.  It's hard on the reviewers and its hard
>> on me too -- with stuff moving around it's hard to easily see that the
>> implementation isn't changing unexpectedly.
>>
>>
>>
>> What I've come up with to hopefully make this dramatically easier is to:
>>
>> 1. Go ahead with creating new .h files for the new classes, but keep the
>> implementations inside tree-vrp.c (for now).
>>
>> 2. Use some delegating member functions to minimize deltas during the
>> transition.  So for example, the evrp class has a getter/setter
>> vr_values, that just delegates to the vr_values class.  This means that
>> an evrp member function can still use get_value_range (for example).
>>
>>
>> #1 and #2 mean that we minimize the textual changes to bring in the new
>> class structure.  Typically we're going to see free functions move into
>> a class hierarchy.  So for example we currently have
>>
>> static value_range *
>> get_value_range (const_tree var)
>> { ... ]
>>
>> That would change to
>>
>> value_range
>> vr_values::get_value_range (const_tree var)
>>
>>
>> With virtually no changes to its body or callers.  That's pretty easy to
>> review.  When there are changes to an implementation they'll be a lot
>> easier to see.  So we get the class structure installed and then proceed to:
>>
>>
>>
>> #3 Pull the member functions out of tree-vrp into their respective new
>> files.  Right now I've got tree-evrp range-analyzer and vr-values for
>> the evrp optimization, generic range analysis and access to vr-values.
>> The names, of course, can certainly change.
>>
>> There'll be some free functions that will need to be shared.  Those
>> routines are context free -- ie, they can be used anytime as they don't
>> access any class or global data.   Everything else will be accessed
>> through the class instances.
>>
>> #4 Consider removal of the delegators.  So for a call within the evrp
>> bits to get_value_range would change from
>>
>>   value_range *vr = get_value_range (op);
>>
>> to
>>
>>   value_range *vr = vr_values.get_value_range (op);
>>
>>
>> We're still calling the same function in both instances. The first just
>> has to go through the delegator.   The second pulls the vr_values
>> instance out of the evrp class and calls the function directly.
>>
>>
>> Thoughts?
> 
> Sounds like a good plan overall.
And I forgot #5.  Enable in DOM and remove threading code in tree-vrp.c
and associated cleanups :-) and #6 enable in sprintf warning pass.

jeff