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

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Nov 8, 2017 at 1:53 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> On Tue, Nov 07, 2017 at 02:03:19PM -0700, 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.
>
> Yeah, this all looks pretty reasonable, ime breaking things also helps
> when writing the patch and you break something since there's less to
> undo before getting to a working state.  If anything I would have broken
> this up even more, moving the existing classes up can probably be its
> own thing, as well as the use of equiv->obstack.

Patch looks good to me.  I saw

+  void set_value_range_ (tree, value_range *);

and when seeing uses I thought temporary hack because of that
"SSA_NAME_VERSION > length" check but then there's no
set_value_range () function (without _) (or I missed it).  Can you just
remove the _ or add a comment?

There's excess vertical space at the end of the vr_values class.

Otherwise ok.  No need to split up further.

Thanks,
Richard.

> Trev
>