public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [RFA][PATCH] Provide a class interface into substitute_and_fold.
@ 2017-10-24 18:57 Jeff Law
  2017-10-24 21:11 ` David Malcolm
  2017-10-26  9:29 ` Richard Biener
  0 siblings, 2 replies; 11+ messages in thread
From: Jeff Law @ 2017-10-24 18:57 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

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



[-- Attachment #2: P2 --]
[-- Type: text/plain, Size: 13596 bytes --]

	* 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
+++ b/gcc/tree-ssa-copy.c
@@ -489,10 +489,16 @@ init_copy_prop (void)
     }
 }
 
+class copy_folder : public substitute_and_fold_engine
+{
+ public:
+  tree get_value (tree);
+};
+
 /* Callback for substitute_and_fold to get at the final copy-of values.  */
 
-static tree
-get_value (tree name)
+tree
+copy_folder::get_value (tree name)
 {
   tree val;
   if (SSA_NAME_VERSION (name) >= n_copy_of)
@@ -557,7 +563,8 @@ fini_copy_prop (void)
 	}
     }
 
-  bool changed = substitute_and_fold (get_value, NULL);
+  class copy_folder copy_folder;
+  bool changed = copy_folder.substitute_and_fold ();
   if (changed)
     {
       free_numbers_of_iterations_estimates (cfun);
diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
index 90df285..62955be 100644
--- a/gcc/tree-ssa-propagate.c
+++ b/gcc/tree-ssa-propagate.c
@@ -853,7 +853,7 @@ static struct prop_stats_d prop_stats;
    PROP_VALUE. Return true if at least one reference was replaced.  */
 
 bool
-replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value)
+substitute_and_fold_engine::replace_uses_in (gimple *stmt)
 {
   bool replaced = false;
   use_operand_p use;
@@ -862,7 +862,7 @@ replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value)
   FOR_EACH_SSA_USE_OPERAND (use, stmt, iter, SSA_OP_USE)
     {
       tree tuse = USE_FROM_PTR (use);
-      tree val = (*get_value) (tuse);
+      tree val = get_value (tuse);
 
       if (val == tuse || val == NULL_TREE)
 	continue;
@@ -891,8 +891,8 @@ replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value)
 /* Replace propagated values into all the arguments for PHI using the
    values from PROP_VALUE.  */
 
-static bool
-replace_phi_args_in (gphi *phi, ssa_prop_get_value_fn get_value)
+bool
+substitute_and_fold_engine::replace_phi_args_in (gphi *phi)
 {
   size_t i;
   bool replaced = false;
@@ -909,7 +909,7 @@ replace_phi_args_in (gphi *phi, ssa_prop_get_value_fn get_value)
 
       if (TREE_CODE (arg) == SSA_NAME)
 	{
-	  tree val = (*get_value) (arg);
+	  tree val = get_value (arg);
 
 	  if (val && val != arg && may_propagate_copy (arg, val))
 	    {
@@ -960,10 +960,10 @@ class substitute_and_fold_dom_walker : public dom_walker
 {
 public:
     substitute_and_fold_dom_walker (cdi_direction direction,
-				    ssa_prop_get_value_fn get_value_fn_,
-				    ssa_prop_fold_stmt_fn fold_fn_)
-	: dom_walker (direction), get_value_fn (get_value_fn_),
-      fold_fn (fold_fn_), something_changed (false)
+				    class substitute_and_fold_engine *engine)
+	: dom_walker (direction),
+          something_changed (false),
+	  substitute_and_fold_engine (engine)
     {
       stmts_to_remove.create (0);
       stmts_to_fixup.create (0);
@@ -979,12 +979,12 @@ public:
     virtual edge before_dom_children (basic_block);
     virtual void after_dom_children (basic_block) {}
 
-    ssa_prop_get_value_fn get_value_fn;
-    ssa_prop_fold_stmt_fn fold_fn;
     bool something_changed;
     vec<gimple *> stmts_to_remove;
     vec<gimple *> stmts_to_fixup;
     bitmap need_eh_cleanup;
+
+    class substitute_and_fold_engine *substitute_and_fold_engine;
 };
 
 edge
@@ -1001,7 +1001,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
 	continue;
       if (res && TREE_CODE (res) == SSA_NAME)
 	{
-	  tree sprime = get_value_fn (res);
+	  tree sprime = substitute_and_fold_engine->get_value (res);
 	  if (sprime
 	      && sprime != res
 	      && may_propagate_copy (res, sprime))
@@ -1010,7 +1010,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
 	      continue;
 	    }
 	}
-      something_changed |= replace_phi_args_in (phi, get_value_fn);
+      something_changed |= substitute_and_fold_engine->replace_phi_args_in (phi);
     }
 
   /* Propagate known values into stmts.  In some case it exposes
@@ -1027,7 +1027,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
       tree lhs = gimple_get_lhs (stmt);
       if (lhs && TREE_CODE (lhs) == SSA_NAME)
 	{
-	  tree sprime = get_value_fn (lhs);
+	  tree sprime = substitute_and_fold_engine->get_value (lhs);
 	  if (sprime
 	      && sprime != lhs
 	      && may_propagate_copy (lhs, sprime)
@@ -1056,7 +1056,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
 			   && gimple_call_noreturn_p (stmt));
 
       /* Replace real uses in the statement.  */
-      did_replace |= replace_uses_in (stmt, get_value_fn);
+      did_replace |= substitute_and_fold_engine->replace_uses_in (stmt);
 
       /* If we made a replacement, fold the statement.  */
       if (did_replace)
@@ -1069,16 +1069,13 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
       /* Some statements may be simplified using propagator
 	 specific information.  Do this before propagating
 	 into the stmt to not disturb pass specific information.  */
-      if (fold_fn)
+      update_stmt_if_modified (stmt);
+      if (substitute_and_fold_engine->fold_stmt(&i))
 	{
-	  update_stmt_if_modified (stmt);
-	  if ((*fold_fn)(&i))
-	    {
-	      did_replace = true;
-	      prop_stats.num_stmts_folded++;
-	      stmt = gsi_stmt (i);
-	      gimple_set_modified (stmt, true);
-	    }
+	  did_replace = true;
+	  prop_stats.num_stmts_folded++;
+	  stmt = gsi_stmt (i);
+	  gimple_set_modified (stmt, true);
 	}
 
       /* If this is a control statement the propagator left edges
@@ -1164,19 +1161,15 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
    Return TRUE when something changed.  */
 
 bool
-substitute_and_fold (ssa_prop_get_value_fn get_value_fn,
-		     ssa_prop_fold_stmt_fn fold_fn)
+substitute_and_fold_engine::substitute_and_fold (void)
 {
-  gcc_assert (get_value_fn);
-
   if (dump_file && (dump_flags & TDF_DETAILS))
     fprintf (dump_file, "\nSubstituting values and folding statements\n\n");
 
   memset (&prop_stats, 0, sizeof (prop_stats));
 
   calculate_dominance_info (CDI_DOMINATORS);
-  substitute_and_fold_dom_walker walker(CDI_DOMINATORS,
-					get_value_fn, fold_fn);
+  substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
 
   /* We cannot remove stmts during the BB walk, especially not release
diff --git a/gcc/tree-ssa-propagate.h b/gcc/tree-ssa-propagate.h
index cff9e53..629ae77 100644
--- a/gcc/tree-ssa-propagate.h
+++ b/gcc/tree-ssa-propagate.h
@@ -61,17 +61,11 @@ enum ssa_prop_result {
 };
 
 
-/* Call-back functions used by the value propagation engine.  */
-typedef bool (*ssa_prop_fold_stmt_fn) (gimple_stmt_iterator *gsi);
-typedef tree (*ssa_prop_get_value_fn) (tree);
-
-
 extern bool valid_gimple_rhs_p (tree);
 extern void move_ssa_defining_stmt_for_defs (gimple *, gimple *);
 extern bool update_gimple_call (gimple_stmt_iterator *, tree, int, ...);
 extern bool update_call_from_tree (gimple_stmt_iterator *, tree);
 extern bool stmt_makes_single_store (gimple *);
-extern bool substitute_and_fold (ssa_prop_get_value_fn, ssa_prop_fold_stmt_fn);
 extern bool may_propagate_copy (tree, tree);
 extern bool may_propagate_copy_into_stmt (gimple *, tree);
 extern bool may_propagate_copy_into_asm (tree);
@@ -79,7 +73,6 @@ extern void propagate_value (use_operand_p, tree);
 extern void replace_exp (use_operand_p, tree);
 extern void propagate_tree_value (tree *, tree);
 extern void propagate_tree_value_into_stmt (gimple_stmt_iterator *, tree);
-extern bool replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value);
 
 /* Public interface into the SSA propagation engine.  Clients should inherit
    from this class and provide their own visitors.  */
@@ -104,4 +97,14 @@ class ssa_propagation_engine
 
 };
 
+class substitute_and_fold_engine
+{
+ public:
+  bool substitute_and_fold (void);
+  bool replace_uses_in (gimple *);
+  virtual bool fold_stmt (gimple_stmt_iterator *) { return false; }
+  virtual tree get_value (tree) { return NULL_TREE; }
+  bool replace_phi_args_in (gphi *);
+};
+
 #endif /* _TREE_SSA_PROPAGATE_H  */
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index e8fce3e..ea8ceee 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -10536,10 +10536,17 @@ fold_predicate_in (gimple_stmt_iterator *si)
   return false;
 }
 
+class vrp_folder : public substitute_and_fold_engine
+{
+ public:
+  tree get_value (tree);
+  bool fold_stmt (gimple_stmt_iterator *);
+};
+
 /* Callback for substitute_and_fold folding the stmt at *SI.  */
 
-static bool
-vrp_fold_stmt (gimple_stmt_iterator *si)
+bool
+vrp_folder::fold_stmt (gimple_stmt_iterator *si)
 {
   if (fold_predicate_in (si))
     return true;
@@ -10547,6 +10554,18 @@ vrp_fold_stmt (gimple_stmt_iterator *si)
   return simplify_stmt_using_ranges (si);
 }
 
+/* If OP has a value range with a single constant value return that,
+   otherwise return NULL_TREE.  This returns OP itself if OP is a
+   constant.
+
+   Implemented as a pure wrapper right now, but this will change.  */
+
+tree
+vrp_folder::get_value (tree op)
+{
+  return op_with_constant_singleton_value_range (op);
+}
+
 /* Return the LHS of any ASSERT_EXPR where OP appears as the first
    argument to the ASSERT_EXPR and in which the ASSERT_EXPR dominates
    BB.  If no such ASSERT_EXPR is found, return OP.  */
@@ -10888,7 +10907,8 @@ vrp_finalize (bool warn_array_bounds_p)
 			  wi::to_wide (vr_value[i]->max));
       }
 
-  substitute_and_fold (op_with_constant_singleton_value_range, vrp_fold_stmt);
+  class vrp_folder vrp_folder;
+  vrp_folder.substitute_and_fold ();
 
   if (warn_array_bounds && warn_array_bounds_p)
     check_all_array_refs ();
@@ -11225,8 +11245,8 @@ evrp_dom_walker::before_dom_children (basic_block bb)
 	}
 
       /* Try folding stmts with the VR discovered.  */
-      bool did_replace
-	= replace_uses_in (stmt, op_with_constant_singleton_value_range);
+      class vrp_folder vrp_folder;
+      bool did_replace = vrp_folder.replace_uses_in (stmt);
       if (fold_stmt (&gsi, follow_single_use_edges)
 	  || did_replace)
 	{

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-24 18:57 [RFA][PATCH] Provide a class interface into substitute_and_fold Jeff Law
@ 2017-10-24 21:11 ` David Malcolm
  2017-10-24 21:47   ` Jeff Law
  2017-10-26  9:29 ` Richard Biener
  1 sibling, 1 reply; 11+ messages in thread
From: David Malcolm @ 2017-10-24 21:11 UTC (permalink / raw)
  To: Jeff Law, gcc-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-24 21:11 ` David Malcolm
@ 2017-10-24 21:47   ` Jeff Law
  2017-10-25 17:23     ` Jeff Law
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff Law @ 2017-10-24 21:47 UTC (permalink / raw)
  To: David Malcolm, gcc-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-24 21:47   ` Jeff Law
@ 2017-10-25 17:23     ` Jeff Law
  2017-10-30  2:26       ` Trevor Saunders
  2017-10-30 10:07       ` Pedro Alves
  0 siblings, 2 replies; 11+ messages in thread
From: Jeff Law @ 2017-10-25 17:23 UTC (permalink / raw)
  To: David Malcolm, gcc-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-24 18:57 [RFA][PATCH] Provide a class interface into substitute_and_fold Jeff Law
  2017-10-24 21:11 ` David Malcolm
@ 2017-10-26  9:29 ` Richard Biener
  2017-10-26 16:55   ` Jeff Law
  1 sibling, 1 reply; 11+ messages in thread
From: Richard Biener @ 2017-10-26  9:29 UTC (permalink / raw)
  To: Jeff Law; +Cc: gcc-patches

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.

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 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
> +++ b/gcc/tree-ssa-copy.c
> @@ -489,10 +489,16 @@ init_copy_prop (void)
>      }
>  }
>
> +class copy_folder : public substitute_and_fold_engine
> +{
> + public:
> +  tree get_value (tree);
> +};
> +
>  /* Callback for substitute_and_fold to get at the final copy-of values.  */
>
> -static tree
> -get_value (tree name)
> +tree
> +copy_folder::get_value (tree name)
>  {
>    tree val;
>    if (SSA_NAME_VERSION (name) >= n_copy_of)
> @@ -557,7 +563,8 @@ fini_copy_prop (void)
>         }
>      }
>
> -  bool changed = substitute_and_fold (get_value, NULL);
> +  class copy_folder copy_folder;
> +  bool changed = copy_folder.substitute_and_fold ();
>    if (changed)
>      {
>        free_numbers_of_iterations_estimates (cfun);
> diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
> index 90df285..62955be 100644
> --- a/gcc/tree-ssa-propagate.c
> +++ b/gcc/tree-ssa-propagate.c
> @@ -853,7 +853,7 @@ static struct prop_stats_d prop_stats;
>     PROP_VALUE. Return true if at least one reference was replaced.  */
>
>  bool
> -replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value)
> +substitute_and_fold_engine::replace_uses_in (gimple *stmt)
>  {
>    bool replaced = false;
>    use_operand_p use;
> @@ -862,7 +862,7 @@ replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value)
>    FOR_EACH_SSA_USE_OPERAND (use, stmt, iter, SSA_OP_USE)
>      {
>        tree tuse = USE_FROM_PTR (use);
> -      tree val = (*get_value) (tuse);
> +      tree val = get_value (tuse);
>
>        if (val == tuse || val == NULL_TREE)
>         continue;
> @@ -891,8 +891,8 @@ replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value)
>  /* Replace propagated values into all the arguments for PHI using the
>     values from PROP_VALUE.  */
>
> -static bool
> -replace_phi_args_in (gphi *phi, ssa_prop_get_value_fn get_value)
> +bool
> +substitute_and_fold_engine::replace_phi_args_in (gphi *phi)
>  {
>    size_t i;
>    bool replaced = false;
> @@ -909,7 +909,7 @@ replace_phi_args_in (gphi *phi, ssa_prop_get_value_fn get_value)
>
>        if (TREE_CODE (arg) == SSA_NAME)
>         {
> -         tree val = (*get_value) (arg);
> +         tree val = get_value (arg);
>
>           if (val && val != arg && may_propagate_copy (arg, val))
>             {
> @@ -960,10 +960,10 @@ class substitute_and_fold_dom_walker : public dom_walker
>  {
>  public:
>      substitute_and_fold_dom_walker (cdi_direction direction,
> -                                   ssa_prop_get_value_fn get_value_fn_,
> -                                   ssa_prop_fold_stmt_fn fold_fn_)
> -       : dom_walker (direction), get_value_fn (get_value_fn_),
> -      fold_fn (fold_fn_), something_changed (false)
> +                                   class substitute_and_fold_engine *engine)
> +       : dom_walker (direction),
> +          something_changed (false),
> +         substitute_and_fold_engine (engine)
>      {
>        stmts_to_remove.create (0);
>        stmts_to_fixup.create (0);
> @@ -979,12 +979,12 @@ public:
>      virtual edge before_dom_children (basic_block);
>      virtual void after_dom_children (basic_block) {}
>
> -    ssa_prop_get_value_fn get_value_fn;
> -    ssa_prop_fold_stmt_fn fold_fn;
>      bool something_changed;
>      vec<gimple *> stmts_to_remove;
>      vec<gimple *> stmts_to_fixup;
>      bitmap need_eh_cleanup;
> +
> +    class substitute_and_fold_engine *substitute_and_fold_engine;
>  };
>
>  edge
> @@ -1001,7 +1001,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
>         continue;
>        if (res && TREE_CODE (res) == SSA_NAME)
>         {
> -         tree sprime = get_value_fn (res);
> +         tree sprime = substitute_and_fold_engine->get_value (res);
>           if (sprime
>               && sprime != res
>               && may_propagate_copy (res, sprime))
> @@ -1010,7 +1010,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
>               continue;
>             }
>         }
> -      something_changed |= replace_phi_args_in (phi, get_value_fn);
> +      something_changed |= substitute_and_fold_engine->replace_phi_args_in (phi);
>      }
>
>    /* Propagate known values into stmts.  In some case it exposes
> @@ -1027,7 +1027,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
>        tree lhs = gimple_get_lhs (stmt);
>        if (lhs && TREE_CODE (lhs) == SSA_NAME)
>         {
> -         tree sprime = get_value_fn (lhs);
> +         tree sprime = substitute_and_fold_engine->get_value (lhs);
>           if (sprime
>               && sprime != lhs
>               && may_propagate_copy (lhs, sprime)
> @@ -1056,7 +1056,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
>                            && gimple_call_noreturn_p (stmt));
>
>        /* Replace real uses in the statement.  */
> -      did_replace |= replace_uses_in (stmt, get_value_fn);
> +      did_replace |= substitute_and_fold_engine->replace_uses_in (stmt);
>
>        /* If we made a replacement, fold the statement.  */
>        if (did_replace)
> @@ -1069,16 +1069,13 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
>        /* Some statements may be simplified using propagator
>          specific information.  Do this before propagating
>          into the stmt to not disturb pass specific information.  */
> -      if (fold_fn)
> +      update_stmt_if_modified (stmt);
> +      if (substitute_and_fold_engine->fold_stmt(&i))
>         {
> -         update_stmt_if_modified (stmt);
> -         if ((*fold_fn)(&i))
> -           {
> -             did_replace = true;
> -             prop_stats.num_stmts_folded++;
> -             stmt = gsi_stmt (i);
> -             gimple_set_modified (stmt, true);
> -           }
> +         did_replace = true;
> +         prop_stats.num_stmts_folded++;
> +         stmt = gsi_stmt (i);
> +         gimple_set_modified (stmt, true);
>         }
>
>        /* If this is a control statement the propagator left edges
> @@ -1164,19 +1161,15 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
>     Return TRUE when something changed.  */
>
>  bool
> -substitute_and_fold (ssa_prop_get_value_fn get_value_fn,
> -                    ssa_prop_fold_stmt_fn fold_fn)
> +substitute_and_fold_engine::substitute_and_fold (void)
>  {
> -  gcc_assert (get_value_fn);
> -
>    if (dump_file && (dump_flags & TDF_DETAILS))
>      fprintf (dump_file, "\nSubstituting values and folding statements\n\n");
>
>    memset (&prop_stats, 0, sizeof (prop_stats));
>
>    calculate_dominance_info (CDI_DOMINATORS);
> -  substitute_and_fold_dom_walker walker(CDI_DOMINATORS,
> -                                       get_value_fn, fold_fn);
> +  substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
>    walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>
>    /* We cannot remove stmts during the BB walk, especially not release
> diff --git a/gcc/tree-ssa-propagate.h b/gcc/tree-ssa-propagate.h
> index cff9e53..629ae77 100644
> --- a/gcc/tree-ssa-propagate.h
> +++ b/gcc/tree-ssa-propagate.h
> @@ -61,17 +61,11 @@ enum ssa_prop_result {
>  };
>
>
> -/* Call-back functions used by the value propagation engine.  */
> -typedef bool (*ssa_prop_fold_stmt_fn) (gimple_stmt_iterator *gsi);
> -typedef tree (*ssa_prop_get_value_fn) (tree);
> -
> -
>  extern bool valid_gimple_rhs_p (tree);
>  extern void move_ssa_defining_stmt_for_defs (gimple *, gimple *);
>  extern bool update_gimple_call (gimple_stmt_iterator *, tree, int, ...);
>  extern bool update_call_from_tree (gimple_stmt_iterator *, tree);
>  extern bool stmt_makes_single_store (gimple *);
> -extern bool substitute_and_fold (ssa_prop_get_value_fn, ssa_prop_fold_stmt_fn);
>  extern bool may_propagate_copy (tree, tree);
>  extern bool may_propagate_copy_into_stmt (gimple *, tree);
>  extern bool may_propagate_copy_into_asm (tree);
> @@ -79,7 +73,6 @@ extern void propagate_value (use_operand_p, tree);
>  extern void replace_exp (use_operand_p, tree);
>  extern void propagate_tree_value (tree *, tree);
>  extern void propagate_tree_value_into_stmt (gimple_stmt_iterator *, tree);
> -extern bool replace_uses_in (gimple *stmt, ssa_prop_get_value_fn get_value);
>
>  /* Public interface into the SSA propagation engine.  Clients should inherit
>     from this class and provide their own visitors.  */
> @@ -104,4 +97,14 @@ class ssa_propagation_engine
>
>  };
>
> +class substitute_and_fold_engine
> +{
> + public:
> +  bool substitute_and_fold (void);
> +  bool replace_uses_in (gimple *);
> +  virtual bool fold_stmt (gimple_stmt_iterator *) { return false; }
> +  virtual tree get_value (tree) { return NULL_TREE; }
> +  bool replace_phi_args_in (gphi *);
> +};
> +
>  #endif /* _TREE_SSA_PROPAGATE_H  */
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index e8fce3e..ea8ceee 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -10536,10 +10536,17 @@ fold_predicate_in (gimple_stmt_iterator *si)
>    return false;
>  }
>
> +class vrp_folder : public substitute_and_fold_engine
> +{
> + public:
> +  tree get_value (tree);
> +  bool fold_stmt (gimple_stmt_iterator *);
> +};
> +
>  /* Callback for substitute_and_fold folding the stmt at *SI.  */
>
> -static bool
> -vrp_fold_stmt (gimple_stmt_iterator *si)
> +bool
> +vrp_folder::fold_stmt (gimple_stmt_iterator *si)
>  {
>    if (fold_predicate_in (si))
>      return true;
> @@ -10547,6 +10554,18 @@ vrp_fold_stmt (gimple_stmt_iterator *si)
>    return simplify_stmt_using_ranges (si);
>  }
>
> +/* If OP has a value range with a single constant value return that,
> +   otherwise return NULL_TREE.  This returns OP itself if OP is a
> +   constant.
> +
> +   Implemented as a pure wrapper right now, but this will change.  */
> +
> +tree
> +vrp_folder::get_value (tree op)
> +{
> +  return op_with_constant_singleton_value_range (op);
> +}
> +
>  /* Return the LHS of any ASSERT_EXPR where OP appears as the first
>     argument to the ASSERT_EXPR and in which the ASSERT_EXPR dominates
>     BB.  If no such ASSERT_EXPR is found, return OP.  */
> @@ -10888,7 +10907,8 @@ vrp_finalize (bool warn_array_bounds_p)
>                           wi::to_wide (vr_value[i]->max));
>        }
>
> -  substitute_and_fold (op_with_constant_singleton_value_range, vrp_fold_stmt);
> +  class vrp_folder vrp_folder;
> +  vrp_folder.substitute_and_fold ();
>
>    if (warn_array_bounds && warn_array_bounds_p)
>      check_all_array_refs ();
> @@ -11225,8 +11245,8 @@ evrp_dom_walker::before_dom_children (basic_block bb)
>         }
>
>        /* Try folding stmts with the VR discovered.  */
> -      bool did_replace
> -       = replace_uses_in (stmt, op_with_constant_singleton_value_range);
> +      class vrp_folder vrp_folder;
> +      bool did_replace = vrp_folder.replace_uses_in (stmt);
>        if (fold_stmt (&gsi, follow_single_use_edges)
>           || did_replace)
>         {
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-26  9:29 ` Richard Biener
@ 2017-10-26 16:55   ` Jeff Law
  2017-10-26 18:12     ` Richard Biener
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff Law @ 2017-10-26 16:55 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-26 16:55   ` Jeff Law
@ 2017-10-26 18:12     ` Richard Biener
  2017-10-27 16:24       ` Jeff Law
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Biener @ 2017-10-26 18:12 UTC (permalink / raw)
  To: Jeff Law; +Cc: gcc-patches

On October 26, 2017 6:50:15 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>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.

Good. Nice to see we're in the same boat. 

Richard. 

>Jeff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-26 18:12     ` Richard Biener
@ 2017-10-27 16:24       ` Jeff Law
  0 siblings, 0 replies; 11+ messages in thread
From: Jeff Law @ 2017-10-27 16:24 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 2861 bytes --]

On 10/26/2017 12:11 PM, Richard Biener wrote:
> On October 26, 2017 6:50:15 PM GMT+02:00, Jeff Law <law@redhat.com> 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

[-- Attachment #2: Z --]
[-- Type: text/plain, Size: 3288 bytes --]

commit dca6f665879a75becb08697653a47ced71a627a0
Author: Jeff Law <law@torsion.usersys.redhat.com>
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, &min, &max);
+	  enum value_range_type range_type = x->get_range_info (arg, &min, &max);
 	  if (range_type == VR_RANGE)
 	    {
 	      HOST_WIDE_INT type_min
@@ -1443,7 +1448,7 @@ format_integer (const directive &dir, 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, &min, &max);
+      enum value_range_type range_type = x->get_range_info (arg, &min, &max);
       if (range_type == VR_RANGE)
 	{
 	  argmin = wide_int_to_tree (argtype, min);
@@ -3883,7 +3888,7 @@ sprintf_dom_walker::handle_gimple_call (gimple_stmt_iterator *gsi)
 	     of them at level 2.  */
 	  wide_int min, max;
 	  enum value_range_type range_type
-	    = get_range_info (size, &min, &max);
+	    = x->get_range_info (size, &min, &max);
 	  if (range_type == VR_RANGE)
 	    {
 	      dstsize
@@ -3995,6 +4000,7 @@ sprintf_dom_walker::handle_gimple_call (gimple_stmt_iterator *gsi)
 edge
 sprintf_dom_walker::before_dom_children (basic_block bb)
 {
+  range_analyzer.enter (bb);
   for (gimple_stmt_iterator si = gsi_start_bb (bb); !gsi_end_p (si); )
     {
       /* Iterate over statements, looking for function calls.  */
@@ -4010,6 +4016,12 @@ sprintf_dom_walker::before_dom_children (basic_block bb)
   return NULL;
 }
 
+void
+sprintf_dom_walker::after_dom_children (basic_block bb)
+{
+  range_analyzer.leave (bb);
+}
+
 /* Execute the pass for function FUN.  */
 
 unsigned int
@@ -4020,6 +4032,7 @@ pass_sprintf_length::execute (function *fun)
   calculate_dominance_info (CDI_DOMINATORS);
 
   sprintf_dom_walker sprintf_dom_walker;
+  x = &sprintf_dom_walker.range_analyzer.vr_values;
   sprintf_dom_walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
 
   /* Clean up object size info.  */

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-25 17:23     ` Jeff Law
@ 2017-10-30  2:26       ` Trevor Saunders
  2017-10-30 10:07       ` Pedro Alves
  1 sibling, 0 replies; 11+ messages in thread
From: Trevor Saunders @ 2017-10-30  2:26 UTC (permalink / raw)
  To: Jeff Law; +Cc: David Malcolm, gcc-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-25 17:23     ` Jeff Law
  2017-10-30  2:26       ` Trevor Saunders
@ 2017-10-30 10:07       ` Pedro Alves
  2017-10-31  4:48         ` Jeff Law
  1 sibling, 1 reply; 11+ messages in thread
From: Pedro Alves @ 2017-10-30 10:07 UTC (permalink / raw)
  To: Jeff Law, David Malcolm, gcc-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
  2017-10-30 10:07       ` Pedro Alves
@ 2017-10-31  4:48         ` Jeff Law
  0 siblings, 0 replies; 11+ messages in thread
From: Jeff Law @ 2017-10-31  4:48 UTC (permalink / raw)
  To: Pedro Alves, David Malcolm, gcc-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-10-31  4:32 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-24 18:57 [RFA][PATCH] Provide a class interface into substitute_and_fold Jeff Law
2017-10-24 21:11 ` David Malcolm
2017-10-24 21:47   ` Jeff Law
2017-10-25 17:23     ` Jeff Law
2017-10-30  2:26       ` Trevor Saunders
2017-10-30 10:07       ` Pedro Alves
2017-10-31  4:48         ` Jeff Law
2017-10-26  9:29 ` Richard Biener
2017-10-26 16:55   ` Jeff Law
2017-10-26 18:12     ` Richard Biener
2017-10-27 16:24       ` Jeff Law

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).