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

[Jeff Law]

On 10/30/2017 03:32 AM, Pedro Alves wrote:
> On 10/25/2017 06:20 PM, Jeff Law wrote:
> 
>> My conclusion on the virtual dtor issue is that it's not strictly needed
>> right  now.
>>
>> IIUC the issue is you could do something like
>>
>> base *foo = new derived ();
>> [ ... ]
>> delete foo;
>>
>> If the base's destructor is not virtual and foo is a base * pointing to
>> a derived object then the deletion invokes undefined behavior.
>>
>> In theory we shouldn't be doing such things :-)  In fact, if there's a
>> way to prevent this with some magic on the base class I'd love to know
>> about it.
> 
> There is: make the base class destructor protected.
I was just reviewing our coding conventions, mostly because I thought we
had something about using protected.  While doing so I see that our
conventions explicitly indicate that a polymorphic class should have a
virtual destructor.

So even though there's another solution and I don't think these objects
are likely to suffer from the problems noted above, I'm just going to
change things to follow our existing conventions -- just because I'm not
dynamically allocating these objects doesn't mean someone else won't later..


Jeff

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

[Pedro Alves]

On 10/25/2017 06:20 PM, Jeff Law wrote:

> My conclusion on the virtual dtor issue is that it's not strictly needed
> right  now.
> 
> IIUC the issue is you could do something like
> 
> base *foo = new derived ();
> [ ... ]
> delete foo;
> 
> If the base's destructor is not virtual and foo is a base * pointing to
> a derived object then the deletion invokes undefined behavior.
> 
> In theory we shouldn't be doing such things :-)  In fact, if there's a
> way to prevent this with some magic on the base class I'd love to know
> about it.

There is: make the base class destructor protected.

Thanks,
Pedro Alves

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

[Trevor Saunders]

On Wed, Oct 25, 2017 at 11:20:38AM -0600, Jeff Law wrote:
> On 10/24/2017 03:45 PM, Jeff Law wrote:
> > On 10/24/2017 02:57 PM, David Malcolm wrote:
> >> On Tue, 2017-10-24 at 12:44 -0600, Jeff Law 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?
> >>>
> >>> Jeff
> >>>
> >>
> >> [...snip...] 
> >>
> >>> diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
> >>> index fec562e..da06172 100644
> >>> --- a/gcc/tree-ssa-ccp.c
> >>> +++ b/gcc/tree-ssa-ccp.c
> >>> @@ -188,7 +188,6 @@ static ccp_prop_value_t *const_val;
> >>>  static unsigned n_const_val;
> >>>  
> >>>  static void canonicalize_value (ccp_prop_value_t *);
> >>> -static bool ccp_fold_stmt (gimple_stmt_iterator *);
> >>>  static void ccp_lattice_meet (ccp_prop_value_t *, ccp_prop_value_t
> >> *);
> >>>  
> >>>  /* Dump constant propagation value VAL to file OUTF prefixed by
> >> PREFIX.  */
> >>> @@ -909,6 +908,24 @@ do_dbg_cnt (void)
> >>>  }
> >>>  
> >>>  
> >>> +/* We want to provide our own GET_VALUE and FOLD_STMT virtual
> >> methods.  */
> >>> +class ccp_folder : public substitute_and_fold_engine
> >>> +{
> >>> + public:
> >>> +  tree get_value (tree);
> >>> +  bool fold_stmt (gimple_stmt_iterator *);
> >>> +};
> >>
> >> Should these two methods be marked OVERRIDE? (or indeed "FINAL
> >> OVERRIDE"?)  This tells the human reader that they're vfuncs, and C++11
> >> onwards can issue a warning if for some reason they stop being vfuncs
> >> (e.g. the type signature changes somewhere).
> >>
> >> (likewise for all the other overrides in subclasses of s_a_f_engine).
> > OVERRIDE seems like a no-brainer.  I can't offhand think of a case where
> > we'd want to derive further.  FINAL (in theory) ought to help divirt, so
> > it seems appropriate as well.  Consider that done :-)
> I added both and went through the usual bootstrap and regression
> testing.  No issues (as expected).  Good to get the confirmation though.
> 
> > 
> > I'm also investigating if these classes need a virtual dtor.
> My conclusion on the virtual dtor issue is that it's not strictly needed
> right  now.
> 
> IIUC the issue is you could do something like
> 
> base *foo = new derived ();
> [ ... ]
> delete foo;
> 
> If the base's destructor is not virtual and foo is a base * pointing to
> a derived object then the deletion invokes undefined behavior.
> 
> In theory we shouldn't be doing such things :-)  In fact, if there's a
> way to prevent this with some magic on the base class I'd love to know
> about it.

-Wdelete-non-virtual-dtor (enabled by -Wall) already checks for cases
that might be problematic.  It seems like most of the time these classes
are on the stack so you never call delete and so won't get warnings.
You can also mark classes as final, if the type you call delete on can't
be inherited from then it must be safe to directly call its dtor.

Trev


> 
> jeff

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

[Jeff Law]

On 10/26/2017 12:11 PM, Richard Biener wrote:
> On October 26, 2017 6:50:15 PM GMT+02:00, Jeff Law  wrote:

[ Big snip ]

>>> 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. 
And just so folks can see where this is going.  This is what it takes to
embed a vrp-style range analyzer into the sprintf warnings pass.

There's a #include to pick up the range analyzer.  A new override in the
sprintf dom walker and a new data member in the sprintf dom walker.

You can see the calls into the range analyzer, one in the
before_dom_children override and the other in the after_dom_children
override to generate the context sensitive range information.  There's 3
calls into a class method to get the range info.

That's the general procedure for any dom walker that we'd want to have
access to context sensitive range data.  #include, new data member in
the walker class,  an enter/exit call in the {before,after}_dom_children
overrides and redirecting the queries into the range analyzer rather
than querying the global info.



Two warts in the current implementation:

First, the queries into the range analyzer occur deeper into the call
chain, so we don't have trivial access to the analyzer class instance.
So for now I just shove it into a global.

This is a common issue I see with our codebase -- we may have some of
the higher level objects in place, but when it comes to accessing those
objects deeper in the call chains, we punt to globals.  It's probably as
much an artifact of our history as a C project as anything.

Truly class-ifying the code or passing down the class instance sometimes
has little/no benefit and we stop and punt to a global.  That's probably
not an unreasonable decision, particularly in self-contained code.  But
once we want to re-use the code it's a major hindrance.  In fact, the
whole process around cleaning up vr_data in tree-vrp is another instance
of this exact issue.   Anyway, I'll try to avoid ranting too much
because I'm as guilty as anyone on this issue.

Second.  The name of the method to get the range information from the
analyzer is the same as the global scoped function to get range
information from the SSA_NAME.  That opens us up to shadowing problems.
Obviously changing the name of one or the other will need to happen.

Anyway, the idea is to show the general procedure for adding range
analysis to a domwalker based optimization/analysis pass.

Anyway, back to cleaning up vr_values :-)

Jeff
commit dca6f665879a75becb08697653a47ced71a627a0
Author: Jeff Law 
Date:   Thu Oct 26 14:19:42 2017 -0400

Range analyzer in sprintf code

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 7415413..4ccc140 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -80,6 +80,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "substring-locations.h"
 #include "diagnostic.h"
 #include "domwalk.h"
+#include "range-analyzer.h"
 
 /* The likely worst case value of MB_LEN_MAX for the target, large enough
for UTF-8.  Ideally, this would be obtained by a target hook if it were
@@ -121,12 +122,16 @@ class sprintf_dom_walker : public dom_walker
   ~sprintf_dom_walker () {}
 
   virtual edge before_dom_children (basic_block) FINAL OVERRIDE;
+  virtual void after_dom_children (basic_block) FINAL OVERRIDE;
   bool handle_gimple_call (gimple_stmt_iterator *);
 
   struct call_info;
   bool compute_format_length (call_info &, format_result *);
+  class range_analyzer range_analyzer;
 };
 
+static class vr_values *x;
+
 class pass_sprintf_length : public gimple_opt_pass
 {
   bool fold_return_value;
@@ -1143,7 +1148,7 @@ get_int_range (tree arg, HOST_WIDE_INT *pmin, 
HOST_WIDE_INT *pmax,
{
  /* Try to determine the range of values of the integer argument.  */
  wide_int min, max;
- enum value_range_type range_type = get_range_info (arg, , );
+ enum value_range_type range_type = x->get_range_info (arg, , 
);
  if (range_type == VR_RANGE)
{
  HOST_WIDE_INT type_min
@@ -1443,7 +1448,7 @@ format_integer (const directive , tree arg)
   /* Try to determine the range of values of the integer argument
 (range information is not available for pointers).  */
   wide_int min, max;
-  enum value_range_type range_type = get_range_info (arg, , );
+  enum value_range_type range_type = x->get_range_info (arg, , );
   if (range_type == VR_RANGE)
{
  argmin = wide_int_to_tree (argtype, min);
@@ -3883,7 +3888,7 @@ 

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

[Richard Biener]

On October 26, 2017 6:50:15 PM GMT+02:00, Jeff Law  wrote:
>On 10/26/2017 03:24 AM, Richard Biener wrote:
>> On Tue, Oct 24, 2017 at 8:44 PM, Jeff Law  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 

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

[Jeff Law]

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

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

[Richard Biener]

On Tue, Oct 24, 2017 at 8:44 PM, Jeff Law  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.

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.

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.

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?

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?

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.

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).

Thanks,
Richard.

> Jeff
>
>
>
> * tree-ssa-ccp.c (ccp_folder): New class derived from
> substitute_and_fold_engine.
> (ccp_folder::get_value): New member function.
> (ccp_folder::fold_stmt): Renamed from ccp_fold_stmt.
> (ccp_fold_stmt): Remove prototype.
> (ccp_finalize): Call substitute_and_fold from the ccp_class.
> * tree-ssa-copy.c (copy_folder): New class derived from
> substitute_and_fold_engine.
> (copy_folder::get_value): Renamed from get_value.
> (fini_copy_prop): Call substitute_and_fold from copy_folder class.
> * tree-vrp.c (vrp_folder): New class derived from
> substitute_and_fold_engine.
> (vrp_folder::fold_stmt): Renamed from vrp_fold_stmt.
> (vrp_folder::get_value): New member function.
> (vrp_finalize): Call substitute_and_fold from vrp_folder class.
> (evrp_dom_walker::before_dom_children): Similarly for replace_uses_in.
> * tree-ssa-propagate.h (substitute_and_fold_engine): New class to
> provide a class interface to folder/substitute routines.
> (ssa_prop_fold_stmt_fn): Remove typedef.
> (ssa_prop_get_value_fn): Likewise.
> (subsitute_and_fold): Remove prototype.
> (replace_uses_in): Likewise.
> * tree-ssa-propagate.c (substitute_and_fold_engine::replace_uses_in):
> Renamed from replace_uses_in.  Call the virtual member function
> (substitute_and_fold_engine::replace_phi_args_in): Similarly.
> (substitute_and_fold_dom_walker): Remove initialization of
> data member entries for calbacks.  Add substitute_and_fold_engine
> member and initialize it.
> (substitute_and_fold_dom_walker::before_dom_children0: Use the
> member functions for get_value, replace_phi_args_in c
> replace_uses_in, and fold_stmt calls.
> (substitute_and_fold_engine::substitute_and_fold): Renamed from
> substitute_and_fold.  Remove assert.   Update ctor call.
>
>
> diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
> index fec562e..da06172 100644
> --- a/gcc/tree-ssa-ccp.c
> +++ b/gcc/tree-ssa-ccp.c
> @@ -188,7 +188,6 @@ static ccp_prop_value_t *const_val;
>  static unsigned n_const_val;
>
>  static void canonicalize_value (ccp_prop_value_t *);
> -static bool ccp_fold_stmt (gimple_stmt_iterator *);
>  static void 

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

[Jeff Law]

On 10/24/2017 03:45 PM, Jeff Law wrote:
> On 10/24/2017 02:57 PM, David Malcolm wrote:
>> On Tue, 2017-10-24 at 12:44 -0600, Jeff Law 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?
>>>
>>> Jeff
>>>
>>
>> [...snip...] 
>>
>>> diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
>>> index fec562e..da06172 100644
>>> --- a/gcc/tree-ssa-ccp.c
>>> +++ b/gcc/tree-ssa-ccp.c
>>> @@ -188,7 +188,6 @@ static ccp_prop_value_t *const_val;
>>>  static unsigned n_const_val;
>>>  
>>>  static void canonicalize_value (ccp_prop_value_t *);
>>> -static bool ccp_fold_stmt (gimple_stmt_iterator *);
>>>  static void ccp_lattice_meet (ccp_prop_value_t *, ccp_prop_value_t
>> *);
>>>  
>>>  /* Dump constant propagation value VAL to file OUTF prefixed by
>> PREFIX.  */
>>> @@ -909,6 +908,24 @@ do_dbg_cnt (void)
>>>  }
>>>  
>>>  
>>> +/* We want to provide our own GET_VALUE and FOLD_STMT virtual
>> methods.  */
>>> +class ccp_folder : public substitute_and_fold_engine
>>> +{
>>> + public:
>>> +  tree get_value (tree);
>>> +  bool fold_stmt (gimple_stmt_iterator *);
>>> +};
>>
>> Should these two methods be marked OVERRIDE? (or indeed "FINAL
>> OVERRIDE"?)  This tells the human reader that they're vfuncs, and C++11
>> onwards can issue a warning if for some reason they stop being vfuncs
>> (e.g. the type signature changes somewhere).
>>
>> (likewise for all the other overrides in subclasses of s_a_f_engine).
> OVERRIDE seems like a no-brainer.  I can't offhand think of a case where
> we'd want to derive further.  FINAL (in theory) ought to help divirt, so
> it seems appropriate as well.  Consider that done :-)
I added both and went through the usual bootstrap and regression
testing.  No issues (as expected).  Good to get the confirmation though.

> 
> I'm also investigating if these classes need a virtual dtor.
My conclusion on the virtual dtor issue is that it's not strictly needed
right  now.

IIUC the issue is you could do something like

base *foo = new derived ();
[ ... ]
delete foo;

If the base's destructor is not virtual and foo is a base * pointing to
a derived object then the deletion invokes undefined behavior.

In theory we shouldn't be doing such things :-)  In fact, if there's a
way to prevent this with some magic on the base class I'd love to know
about it.

jeff

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

[Jeff Law]

On 10/24/2017 02:57 PM, David Malcolm wrote:
> On Tue, 2017-10-24 at 12:44 -0600, Jeff Law 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?
>>
>> Jeff
>>
> 
> [...snip...] 
> 
>> diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
>> index fec562e..da06172 100644
>> --- a/gcc/tree-ssa-ccp.c
>> +++ b/gcc/tree-ssa-ccp.c
>> @@ -188,7 +188,6 @@ static ccp_prop_value_t *const_val;
>>  static unsigned n_const_val;
>>  
>>  static void canonicalize_value (ccp_prop_value_t *);
>> -static bool ccp_fold_stmt (gimple_stmt_iterator *);
>>  static void ccp_lattice_meet (ccp_prop_value_t *, ccp_prop_value_t
> *);
>>  
>>  /* Dump constant propagation value VAL to file OUTF prefixed by
> PREFIX.  */
>> @@ -909,6 +908,24 @@ do_dbg_cnt (void)
>>  }
>>  
>>  
>> +/* We want to provide our own GET_VALUE and FOLD_STMT virtual
> methods.  */
>> +class ccp_folder : public substitute_and_fold_engine
>> +{
>> + public:
>> +  tree get_value (tree);
>> +  bool fold_stmt (gimple_stmt_iterator *);
>> +};
> 
> Should these two methods be marked OVERRIDE? (or indeed "FINAL
> OVERRIDE"?)  This tells the human reader that they're vfuncs, and C++11
> onwards can issue a warning if for some reason they stop being vfuncs
> (e.g. the type signature changes somewhere).
> 
> (likewise for all the other overrides in subclasses of s_a_f_engine).
OVERRIDE seems like a no-brainer.  I can't offhand think of a case where
we'd want to derive further.  FINAL (in theory) ought to help divirt, so
it seems appropriate as well.  Consider that done :-)

I'm also investigating if these classes need a virtual dtor.

Jeff

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

[David Malcolm]

On Tue, 2017-10-24 at 12:44 -0600, Jeff Law 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?
> 
> Jeff
> 

[...snip...] 

> diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
> index fec562e..da06172 100644
> --- a/gcc/tree-ssa-ccp.c
> +++ b/gcc/tree-ssa-ccp.c
> @@ -188,7 +188,6 @@ static ccp_prop_value_t *const_val;
>  static unsigned n_const_val;
>  
>  static void canonicalize_value (ccp_prop_value_t *);
> -static bool ccp_fold_stmt (gimple_stmt_iterator *);
>  static void ccp_lattice_meet (ccp_prop_value_t *, ccp_prop_value_t
*);
>  
>  /* Dump constant propagation value VAL to file OUTF prefixed by
PREFIX.  */
> @@ -909,6 +908,24 @@ do_dbg_cnt (void)
>  }
>  
>  
> +/* We want to provide our own GET_VALUE and FOLD_STMT virtual
methods.  */
> +class ccp_folder : public substitute_and_fold_engine
> +{
> + public:
> +  tree get_value (tree);
> +  bool fold_stmt (gimple_stmt_iterator *);
> +};

Should these two methods be marked OVERRIDE? (or indeed "FINAL
OVERRIDE"?)  This tells the human reader that they're vfuncs, and C++11
onwards can issue a warning if for some reason they stop being vfuncs
(e.g. the type signature changes somewhere).

(likewise for all the other overrides in subclasses of s_a_f_engine).

[...snip...]

Dave

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

[Jeff Law]

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?

Jeff


* tree-ssa-ccp.c (ccp_folder): New class derived from
substitute_and_fold_engine.
(ccp_folder::get_value): New member function.
(ccp_folder::fold_stmt): Renamed from ccp_fold_stmt.
(ccp_fold_stmt): Remove prototype.
(ccp_finalize): Call substitute_and_fold from the ccp_class.
* tree-ssa-copy.c (copy_folder): New class derived from
substitute_and_fold_engine.
(copy_folder::get_value): Renamed from get_value.
(fini_copy_prop): Call substitute_and_fold from copy_folder class.
* tree-vrp.c (vrp_folder): New class derived from
substitute_and_fold_engine.
(vrp_folder::fold_stmt): Renamed from vrp_fold_stmt.
(vrp_folder::get_value): New member function.
(vrp_finalize): Call substitute_and_fold from vrp_folder class.
(evrp_dom_walker::before_dom_children): Similarly for replace_uses_in.
* tree-ssa-propagate.h (substitute_and_fold_engine): New class to
provide a class interface to folder/substitute routines.
(ssa_prop_fold_stmt_fn): Remove typedef.
(ssa_prop_get_value_fn): Likewise.
(subsitute_and_fold): Remove prototype.
(replace_uses_in): Likewise.
* tree-ssa-propagate.c (substitute_and_fold_engine::replace_uses_in):
Renamed from replace_uses_in.  Call the virtual member function
(substitute_and_fold_engine::replace_phi_args_in): Similarly.
(substitute_and_fold_dom_walker): Remove initialization of
data member entries for calbacks.  Add substitute_and_fold_engine
member and initialize it.
(substitute_and_fold_dom_walker::before_dom_children0: Use the
member functions for get_value, replace_phi_args_in c
replace_uses_in, and fold_stmt calls.
(substitute_and_fold_engine::substitute_and_fold): Renamed from
substitute_and_fold.  Remove assert.   Update ctor call.


diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
index fec562e..da06172 100644
--- a/gcc/tree-ssa-ccp.c
+++ b/gcc/tree-ssa-ccp.c
@@ -188,7 +188,6 @@ static ccp_prop_value_t *const_val;
 static unsigned n_const_val;
 
 static void canonicalize_value (ccp_prop_value_t *);
-static bool ccp_fold_stmt (gimple_stmt_iterator *);
 static void ccp_lattice_meet (ccp_prop_value_t *, ccp_prop_value_t *);
 
 /* Dump constant propagation value VAL to file OUTF prefixed by PREFIX.  */
@@ -909,6 +908,24 @@ do_dbg_cnt (void)
 }
 
 
+/* We want to provide our own GET_VALUE and FOLD_STMT virtual methods.  */
+class ccp_folder : public substitute_and_fold_engine
+{
+ public:
+  tree get_value (tree);
+  bool fold_stmt (gimple_stmt_iterator *);
+};
+
+/* This method just wraps GET_CONSTANT_VALUE for now.  Over time
+   naked calls to GET_CONSTANT_VALUE should be eliminated in favor
+   of calling member functions.  */
+
+tree
+ccp_folder::get_value (tree op)
+{
+  return get_constant_value (op);
+}
+
 /* Do final substitution of propagated values, cleanup the flowgraph and
free allocated storage.  If NONZERO_P, record nonzero bits.
 
@@ -967,7 +984,8 @@ ccp_finalize (bool nonzero_p)
 }
 
   /* Perform substitutions based on the known constant values.  */
-  something_changed = substitute_and_fold (get_constant_value, ccp_fold_stmt);
+  class ccp_folder ccp_folder;
+  something_changed = ccp_folder.substitute_and_fold ();
 
   free (const_val);
   const_val = NULL;
@@ -2176,8 +2194,8 @@ fold_builtin_alloca_with_align (gimple *stmt)
 /* Fold the stmt at *GSI with CCP specific information that propagating
and regular folding does not catch.  */
 
-static bool
-ccp_fold_stmt (gimple_stmt_iterator *gsi)
+bool
+ccp_folder::fold_stmt (gimple_stmt_iterator *gsi)
 {
   gimple *stmt = gsi_stmt (*gsi);
 
diff --git a/gcc/tree-ssa-copy.c b/gcc/tree-ssa-copy.c
index a57535c..e45d188 100644
--- a/gcc/tree-ssa-copy.c
+++