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 *); 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 >
