On October 26, 2017 6:50:15 PM GMT+02:00, Jeff Law <l...@redhat.com> wrote: >On 10/26/2017 03:24 AM, Richard Biener wrote: >> On Tue, Oct 24, 2017 at 8:44 PM, Jeff Law <l...@redhat.com> wrote: >>> This is similar to the introduction of the ssa_propagate_engine, but >for >>> the substitution/replacements bits. >>> >>> In a couple places the pass specific virtual functions are just >wrappers >>> around existing functions. A good example of this is >>> ccp_folder::get_value. Many other routines in tree-ssa-ccp.c want >to >>> use get_constant_value. Some may be convertable to use the class >>> instance, but I haven't looked closely. >>> >>> Another example is vrp_folder::get_value. In this case we're >wrapping >>> op_with_constant_singleton_value. In a later patch that moves into >the >>> to-be-introduced vr_values class so we'll delegate to that class >rather >>> than wrap. >>> >>> FWIW I did look at having a single class for the propagation engine >and >>> the substitution engine. That turned out to be a bit problematical >due >>> to the calls into the substitution engine from the evrp bits which >don't >>> use the propagation engine at all. Given propagation and >substitution >>> are distinct concepts I ultimately decided the cleanest path forward >was >>> to keep the two classes separate. >>> >>> Bootstrapped and regression tested on x86_64. OK for the trunk? >> >> So what I don't understand in this 2 part series is why you put >> substitute-and-fold into a different class. >Good question. They're in different classes because they can and are >used independently. > >For example, tree-complex uses the propagation engine, but not the >substitution engine. EVRP uses the substitution engine, but not the >propagation engine. The standard VRP algorithm uses both engines, but >other than shared data (vr_values), they are independent. CCP and >copy-prop are similar to VRP. Essentially one is a producer, the other >a consumer. > >It might be possible to smash them together, but I'm not sure if that's >wise or not. I do suspect that smashing them together would be easier >once all the other work is done if we were to make that choice. But >composition, multiple inheritance or just passing around the class >instance may be better. I think that's a TBD. > > >> >> This makes it difficult for users to inherit and put the lattice in >> the deriving class as we have the visit routines which will update >> the lattice and the get_value hook which queries it. >Yes. The key issue is the propagation step produces vr_values and the >substitution step consumes vr_values. > >For VRP the way I solve this is to have a vr_values class in the >derived >propagation engine class as well as the derived substitution engine >class. When we're done with propagation we move the class instance >from >the propagation engine to the substitution engine. > >EVRP works similarly except the vr_values starts in the evrp_dom_walker >class, then moves to its substitution engine. > >There's a bit of cleanup to do there in terms of implementation. But >that's the basic model that I'm using right now. It should be fairly >easy to move to a unioned class or multiple inheritance if we so >desired. It shouldn't affect most of what I'm doing now around >encapsulating vr_values. > >> >> So from maintaining the state for the users using a single >> class whould be more appropriate. Of course it seems like >> substitute-and-fold can be used without using the SSA >> propagator itself and the SSA propagator can be used >> without the substitute and fold engine. >Right. THey can and are used independently which is what led to having >independent classes. > > >> >> IIRC we decided against using multiple inheritance? Which >> means a user would put the lattice in the SSA propagation >> engine derived class and do the inheriting via composition >> as member in the substitute_and_fold engine? >Right, we have decided against using multiple inheritance. So rather >than using multiple inheritance I pass the vr_values object. So in my >development tree I have this: > > >class vrp_prop : public ssa_propagation_engine >{ > public: >enum ssa_prop_result visit_stmt (gimple *, edge *, tree *) FINAL >OVERRIDE; > enum ssa_prop_result visit_phi (gphi *) FINAL OVERRIDE; > > /* XXX Drop the indirection through the pointer, not needed. */ > class vr_values *vr_values; >}; > > >class vrp_folder : public substitute_and_fold_engine >{ > public: > tree get_value (tree) FINAL OVERRIDE; > bool fold_stmt (gimple_stmt_iterator *) FINAL OVERRIDE; > class vr_values *vr_values; >}; > >In vrp_finalize: > class vrp_folder vrp_folder; > vrp_folder.vr_values = vr_values; > vrp_folder.substitute_and_fold (); > > >I'm in the process of cleaning this up -- in particular there'll be a >ctor in vrp_folder which will require passing in a vr_values and we'll >be dropping some indirections as well. > >I just went through his exact cleanup yesterday with the separated evrp >style range analyzer and evrp itself. > > >> Your patches keep things simple (aka the lattice and most >> functions are globals), but is composition what you had >> in mind when doing this class-ification? >Yes. I'm still encapsulating vr_values and figuring out how to deal >with the various routines that want to use it. Essentially every >routine has to be evaluated and either move into the classes or get >passed in the vr_values class instance. There's a lot of the latter >right now just to get things building so that I can see all the points >where evrp and vrp are sharing code. > >There's also a lot of routines that want to operate on the range for a >particular object. They can live outside the vr_ranges object since >they're passed the relevant range. I haven't made any decisions on how >I want to handle those. But it's clear that they're going to need to >be >shared across vrp, evrp and passes that use embedded range analysis. > >They could be their own class, but I suspect everything would just be a >static member since they don't need a class instance. They could move >into vr_values as static members. Or we could just keep them as simple >C functions. > > >> >> Just thinking that if we can encapsulate the propagation >> part of all our propagators we should be able to make >> them work on ranges and instantiated by other consumers >> (basically what you want to do for EVRP), like a hypothetical >> static analysis pass. >Right. I think it's the right design direction. It'll certainly >require more teardown and reshuffling, but the work is, IMHO, >definitely >the right direction and hopefully my work will make that task easier as >the goal is to be able to take the different components and trivially >wire them up. > >I'm really focused on trying to get a clean separation of vrp and evrp. >Within evrp separation of analysis from optimization (so that I can >easily embed the analysis bits). At the heart of all this is >encapsulation of the vr_values structure. > > >> >> Both patches look ok to me though it would be nice to >> do the actual composition with a comment that the >> lattices might be moved here (if all workers became >> member functions as well). >I'm actually hoping to post a snapshot of how this will look in a clean >consumer today (sprintf bits). There's enough in place that I can do >that while I continue teasing apart vrp and evrp.
Good. Nice to see we're in the same boat. Richard. >Jeff