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
