Re: [PATCH] 1/n Refactoring tree-vrp.c, step one introducing vr_values class

[Jeff Law <law at redhat dot com>]

On 11/09/2017 11:12 AM, Martin Sebor wrote:
> On 11/07/2017 02:03 PM, Jeff Law wrote:
>> So this is the first step in pulling apart tree-vrp.c.  As outlined in
>> an earlier message the goal of this patch is just to get the vr_values
>> class introduced.  I've tried (and mostly succeeded) in avoiding doing
>> significant cleanups/rewrites at this point to make reviewing this step
>> easier.
>>
>> The vast majority of this patch is just changing functions from free
>> functions to member functions in the appropriate class (primarily
>> vr_values), pulling the appropriate static objects into that class and
>> adding the temporary delegators to minimize diffs at this stage.
>> Exceptions to this:
>>
>> set_value_range changes how we get at the obstack for allocating
>> bitmaps.  This was discussed on-list recently.  Similarly in
>> vrp_intersect_ranges_1.
>>
>> extract_range_from_unary_expr has two implementations.  One is within
>> the vr_values class which calls out to the freestanding function.  Hence
>> the ::extract_range_from_unary_expr.
>>
>> We drop debug_all_value_ranges.  It doesn't make sense in a world where
>> the value ranges are attached to a class instance.  There is still a
>> method to dump all the value ranges within the class instance.
>>
>> The vrp_prop and vrp_folder class definitions have to move to earlier
>> points in the file.
>>
>> We pass a vrp_prop class instance through the gimple walkers using the
>> wi->info field.  check_all_array_refs sets that up and we recover the
>> class instance pointer in the check_array_bounds callback.
>>
>> I wasn't up to converting the gimple folder to classes at this time.  So
>> we set/wipe x_vr_values (file scoped static) around the calls into the
>> gimple walkers and recover the vr_values class instance from x_vr_values
>> within vrp_valueize and vrp_valueize_1.
>>
>> I use a similar hack to recover the vr_values instance in the callback
>> to simplify a statement for jump threading.  This code is on the
>> chopping block -- I expect it all to just disappear shortly.  Similarly
>> I just pass in vr_values to identify_jump_threads.  Again, I expect to
>> be removing all this code shortly and wanted to keep this as simple as
>> possible rather than waste time cleaning up code I'm just going to
>> remove.
>>
>> I did do some simplifications in vrp_finalize.  It wanted to access
>> vr_value and num_vr_values directly.  Now it goes through the
>> get_value_range interface.
>>
>> I introduced a set_value_range_ member function in vr_values, then
>> twiddled a couple evrp routines to use that rather than access vr_value
>> directly.
>>
>> In a few places I didn't bother creating delegators.  The delegators
>> wouldn't have simplified anything.  See execute_vrp and
>> execute_early_vrp.
>>
>> You'll note the vr_values class is large.  I've tried to avoid the
>> temptation to start simplifying stuff and instead try to stick with
>> moving routines into the appropriate class based on the code as it
>> stands right now.  The size of vr_values is probably a good indicator
>> that it's doing too much.
>>
>> As suggested yesterday, I've added various delegator methods to the
>> classes to minimize code churn at this time.  I suspect we're going to
>> want to remove most, if not all of them.  But again, the goal in this
>> patch is to get the class structure in place with minimal amount of
>> collateral damage to make this step easy to review.
>>
>> It's worth noting you see no changes that bleed out of tree-vrp.c and
>> the vast majority of the patch is just changing the function signature
>> so that they're part of the newly created class.  I consider that a
>> positive :-)
> 
> FWIW, I like the "C++-ification" of the API.
> 
> Just a few high-level suggestions:
> 
> If class vr_values is meant to be copyable and assignable it
> should define a copy ctor and assignment operator.  Otherwise
> it should make them private/deleted (whatever is the convention).
> As it is, it looks to me that if a copy is created by accident
> the dtor will have undefined behavior.
I'd actually like to defer finalizing this until a little bit later
(measured in days, not weeks or months).

Ideally we'd be using C++11 and rvalue references so that we could
transfer ownership of the array within the vr_values class as we move
from (in traditional vrp) propagation through the lattice to propagation
into the IL.

What I've got here in another branch slightly different in that the
ctors expect to be passed in the underlying array and we want to define
a private, empty copy ctor.

I'm still pondering whether to push C++11, go with what I've done on a
private branch or something else.  It also plays into generally cleaning
up initialization and finalization.


> 
> Also in vr_values, should all the data members really be public?
> (If the class maintains any invariants I would expect the members
> to be private and accessors provided for them to prevent clients
> from inadvertently breaking the invariants).
The amount of privatization one can do right now is minimal.  I've
chosen to not do lots of rewriting to facilitate privatization of
methods and data at this stage.  There's a little, but I've really tried
to keep this at a minimum to make the review step easier.

Once the class structure is in place and location of implementation is
rationalized it's easy to then start looking at each member with an eye
towards privatization.   I've done some of that here on another branch.

Similarly one the classification and movement of methods is done, it's
easy to go back and review the remaining touch points to identify
targets for further isolation and classification work.

> 
> Consider defining constructors and destructors for the classes
> instead of providing initialize and finialize member functions.
> (If a dtor takes an argument make it a member of the class.)
Absolutely.  Though you'll find that ordering of initialization within
traditional vrp is painful.  Again, I know this because I've done some
of this on another branch.  You have to be careful about when you
instantiate the vr_values class relative to creating of assert_exprs
(which create additional SSA_NAMEs) relative to scev initialization and
initialization of the bb flags (inserting assert_exprs creates new
blocks too).

All this comes into play as one sets up ctors/dtors for the classes.


> 
> Consider renaming the vrp_prop::vr_values data member so that
> it doesn't clash with the class name.  Same in vrp_folder.  There,
> I would also suggest to use a different name than in vrp_prop since
> the former is a pointer and the latter is an object of the type so
> it could get confusing.
Will certainly consider.

I think the general message is yes, we want to do all these things.  But
we need to stage them in as independent cleanups to facilitate the
review process.  It's also the case that this work often becomes easier
once a fair amount of the c++-ification is in-place.

Thanks,
Jeff