On 11/09/2017 08:24 AM, Richard Biener wrote:
> On Wed, Nov 8, 2017 at 1:53 PM, Trevor Saunders <tbsau...@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
