[RFA][PATCH] Provide a class interface into substitute_and_fold.

Jeff Law law@redhat.com
Thu Oct 26 16:55:00 GMT 2017


On 10/26/2017 03:24 AM, Richard Biener wrote:
> On Tue, Oct 24, 2017 at 8:44 PM, Jeff Law <law@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.

Jeff



More information about the Gcc-patches mailing list