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

[Martin Sebor <msebor at gmail dot com>]

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.

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).

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.)

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.

Martin

> Bootstrapped and regression tested on x86_64.
>
> OK for the trunk?
>
>
> Jeff
>
> ps. Next step is to pull the class methods into their own files without
> changing *any* parts of their implementation.  As a bit of a preview,
> tree-vrp.c is currently around 12000 lines.  After moving methods it'll
> be about 7k.  vr-values.c will have around 5k lines.  The split out EVRP
> pass is ~600 which will be further split roughly in half between the
> core of evrp and the reusable range analysis module.