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 08:24 AM, Richard Biener wrote:
> 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 *);
There's a set_value_range free function I didn't want to conflict with:

/* Set value range VR to {T, MIN, MAX, EQUIV}.  */

void
set_value_range (value_range *vr, enum value_range_type t, tree min,
                 tree max, bitmap equiv)

The free function is passed in a VR we want to initialize/set.

THe set_value_range_ member function is used to set an entry in the
vr_values array.  I'll choose a better name for the member function.

> 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.
Will fix.

> 
> Otherwise ok.  No need to split up further.
Funny, I'd split out the two hunks Trevor suggested yesterday.  It's a
nice mindless little task I can have running while I look at other
stuff.  GIven I've already split it up and it's a tiny plus for
bisecting, when I commit, I'll commit the 3 chunks individually.

jeff