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

[Trevor Saunders <tbsaunde at tbsaunde dot org>]

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.

Trev