Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec

[Martin Sebor <msebor@gmail.com>]

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

On 6/23/21 1:43 AM, Richard Biener wrote:
> On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
>>
>> On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote:
>>> On 6/21/21 1:15 AM, Richard Biener wrote:
> [...]
>>>>
>>>> But maybe I'm misunderstanding C++ too much :/
>>>>
>>>> Well, I guess b) from above means auto_vec<> passing to
>>>> vec<> taking functions will need changes?
>>>
>>> Converting an auto_vec object to a vec slices off its data members.
>>> The auto_vec<T, 0> specialization has no data members so that's not
>>> a bug in and of itself, but auto_vec<T, N> does have data members
>>> so that would be a bug.  The risk is not just passing it to
>>> functions by value but also returning it.  That risk was made
>>> worse by the addition of the move ctor.
>>
>> I would agree that the conversion from auto_vec<> to vec<> is
>> questionable, and should get some work at some point, perhaps just
>> passingauto_vec references is good enough, or perhaps there is value in
>> some const_vec view to avoid having to rely on optimizations, I'm not
>> sure without looking more at the usage.
> 
> We do need to be able to provide APIs that work with both auto_vec<>
> and vec<>, I agree that those currently taking a vec<> by value are
> fragile (and we've had bugs there before), but I'm not ready to say
> that changing them all to [const] vec<>& is OK.  The alternative
> would be passing a const_vec<> by value, passing that along to
> const vec<>& APIs should be valid then (I can see quite some API
> boundary cleanups being necessary here ...).
> 
> But with all this I don't know how to adjust auto_vec<> to no
> longer "decay" to vec<> but still being able to pass it to vec<>&
> and being able to call vec<> member functions w/o jumping through
> hoops.  Any hints on that?  private inheritance achieves the first
> but also hides all the API ...

The simplest way to do that is by preventing the implicit conversion
between the two types and adding a to_vec() member function to
auto_vec as Jason suggested.  This exposes the implicit conversions
to the base vec, forcing us to review each and "fix" it either by
changing the argument to either vec& or const vec&, or less often
to avoid the indirection if necessary, by converting the auto_vec
to vec explicitly by calling to_vec().  The attached diff shows
a very rough POC of what  that might look like.  A side benefit
is that it improves const-safety and makes the effects of functions
on their callers easier to understand.

But that's orthogonal to making auto_vec copy constructible and copy
assignable.  That change can be made independently and with much less
effort and no risk.

Martin

[-- Attachment #2: vec.diff --]
[-- Type: text/x-patch, Size: 13678 bytes --]

diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index a671a3eb740..ab6db3860f5 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -759,8 +759,9 @@ extern tree c_finish_omp_clauses (tree, enum c_omp_region_type);
 extern tree c_build_va_arg (location_t, tree, location_t, tree);
 extern tree c_finish_transaction (location_t, tree, int);
 extern bool c_tree_equal (tree, tree);
-extern tree c_build_function_call_vec (location_t, vec<location_t>, tree,
-				       vec<tree, va_gc> *, vec<tree, va_gc> *);
+extern tree c_build_function_call_vec (location_t, const vec<location_t>&,
+				       tree, vec<tree, va_gc> *,
+				       vec<tree, va_gc> *);
 extern tree c_omp_clause_copy_ctor (tree, tree, tree);
 
 /* Set to 0 at beginning of a function definition, set to 1 if
diff --git a/gcc/dominance.c b/gcc/dominance.c
index 6a262ce8283..cc63391a39a 100644
--- a/gcc/dominance.c
+++ b/gcc/dominance.c
@@ -1227,7 +1227,7 @@ recompute_dominator (enum cdi_direction dir, basic_block bb)
    from BBS.  */
 
 static void
-prune_bbs_to_update_dominators (vec<basic_block> bbs,
+prune_bbs_to_update_dominators (vec<basic_block> &bbs,
 				bool conservative)
 {
   unsigned i;
@@ -1379,7 +1379,7 @@ determine_dominators_for_sons (struct graph *g, vec<basic_block> bbs,
    a block of BBS in the current dominance tree dominate it.  */
 
 void
-iterate_fix_dominators (enum cdi_direction dir, vec<basic_block> bbs,
+iterate_fix_dominators (enum cdi_direction dir, vec<basic_block> &bbs,
 			bool conservative)
 {
   unsigned i;
diff --git a/gcc/dominance.h b/gcc/dominance.h
index 1a8c248ee98..970da02c594 100644
--- a/gcc/dominance.h
+++ b/gcc/dominance.h
@@ -78,7 +78,7 @@ checking_verify_dominators (cdi_direction dir)
 
 basic_block recompute_dominator (enum cdi_direction, basic_block);
 extern void iterate_fix_dominators (enum cdi_direction,
-				    vec<basic_block> , bool);
+				    vec<basic_block> &, bool);
 extern void add_to_dominance_info (enum cdi_direction, basic_block);
 extern void delete_from_dominance_info (enum cdi_direction, basic_block);
 extern basic_block first_dom_son (enum cdi_direction, basic_block);
diff --git a/gcc/genautomata.c b/gcc/genautomata.c
index 6bbfc684afa..e488c5f28ef 100644
--- a/gcc/genautomata.c
+++ b/gcc/genautomata.c
@@ -6137,7 +6137,7 @@ evaluate_equiv_classes (automaton_t automaton, vec<state_t> *equiv_classes)
 
 /* The function merges equivalent states of AUTOMATON.  */
 static void
-merge_states (automaton_t automaton, vec<state_t> equiv_classes)
+merge_states (automaton_t automaton, const vec<state_t> &equiv_classes)
 {
   state_t curr_state;
   state_t new_state;
diff --git a/gcc/genextract.c b/gcc/genextract.c
index 6fe4a2524fc..3ed2f6846c9 100644
--- a/gcc/genextract.c
+++ b/gcc/genextract.c
@@ -214,7 +214,7 @@ VEC_safe_set_locstr (md_rtx_info *info, vec<locstr> *vp,
 /* Another helper subroutine of walk_rtx: given a vec<char>, convert it
    to a NUL-terminated string in malloc memory.  */
 static char *
-VEC_char_to_string (vec<char> v)
+VEC_char_to_string (const vec<char> &v)
 {
   size_t n = v.length ();
   char *s = XNEWVEC (char, n + 1);
diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c
index 632947950e4..02ce068d9cf 100644
--- a/gcc/gimple-ssa-store-merging.c
+++ b/gcc/gimple-ssa-store-merging.c
@@ -2654,7 +2654,8 @@ gather_bswap_load_refs (vec<tree> *refs, tree val)
    go after the = _5 store and thus change behavior.  */
 
 static bool
-check_no_overlap (vec<store_immediate_info *> m_store_info, unsigned int i,
+check_no_overlap (const vec<store_immediate_info *> &m_store_info,
+		  unsigned int i,
 		  bool all_integer_cst_p, unsigned int first_order,
 		  unsigned int last_order, unsigned HOST_WIDE_INT start,
 		  unsigned HOST_WIDE_INT end, unsigned int first_earlier,
diff --git a/gcc/gimple.c b/gcc/gimple.c
index f1044e9c630..108daeda43b 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -241,7 +241,7 @@ gimple_build_call_1 (tree fn, unsigned nargs)
    specified in vector ARGS.  */
 
 gcall *
-gimple_build_call_vec (tree fn, vec<tree> args)
+gimple_build_call_vec (tree fn, const vec<tree> &args)
 {
   unsigned i;
   unsigned nargs = args.length ();
@@ -338,7 +338,7 @@ gimple_build_call_internal (enum internal_fn fn, unsigned nargs, ...)
    specified in vector ARGS.  */
 
 gcall *
-gimple_build_call_internal_vec (enum internal_fn fn, vec<tree> args)
+gimple_build_call_internal_vec (enum internal_fn fn, const vec<tree> &args)
 {
   unsigned i, nargs;
   gcall *call;
@@ -802,7 +802,7 @@ gimple_build_switch_nlabels (unsigned nlabels, tree index, tree default_label)
    ARGS is a vector of labels excluding the default.  */
 
 gswitch *
-gimple_build_switch (tree index, tree default_label, vec<tree> args)
+gimple_build_switch (tree index, tree default_label, const vec<tree> &args)
 {
   unsigned i, nlabels = args.length ();
 
@@ -3049,7 +3049,7 @@ compare_case_labels (const void *p1, const void *p2)
 /* Sort the case labels in LABEL_VEC in place in ascending order.  */
 
 void
-sort_case_labels (vec<tree> label_vec)
+sort_case_labels (vec<tree> &label_vec)
 {
   label_vec.qsort (compare_case_labels);
 }
@@ -3074,7 +3074,7 @@ sort_case_labels (vec<tree> label_vec)
    found or not.  */
 
 void
-preprocess_case_label_vec_for_gimple (vec<tree> labels,
+preprocess_case_label_vec_for_gimple (vec<tree> &labels,
 				      tree index_type,
 				      tree *default_casep)
 {
diff --git a/gcc/gimple.h b/gcc/gimple.h
index e7dc2a45a13..aabf68eaea0 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1516,11 +1516,11 @@ void gimple_init (gimple *g, enum gimple_code code, unsigned num_ops);
 gimple *gimple_alloc (enum gimple_code, unsigned CXX_MEM_STAT_INFO);
 greturn *gimple_build_return (tree);
 void gimple_call_reset_alias_info (gcall *);
-gcall *gimple_build_call_vec (tree, vec<tree> );
+gcall *gimple_build_call_vec (tree, const vec<tree> &);
 gcall *gimple_build_call (tree, unsigned, ...);
 gcall *gimple_build_call_valist (tree, unsigned, va_list);
 gcall *gimple_build_call_internal (enum internal_fn, unsigned, ...);
-gcall *gimple_build_call_internal_vec (enum internal_fn, vec<tree> );
+gcall *gimple_build_call_internal_vec (enum internal_fn, const vec<tree> &);
 gcall *gimple_build_call_from_tree (tree, tree);
 gassign *gimple_build_assign (tree, tree CXX_MEM_STAT_INFO);
 gassign *gimple_build_assign (tree, enum tree_code,
@@ -1547,7 +1547,7 @@ gtry *gimple_build_try (gimple_seq, gimple_seq,
 gimple *gimple_build_wce (gimple_seq);
 gresx *gimple_build_resx (int);
 gswitch *gimple_build_switch_nlabels (unsigned, tree, tree);
-gswitch *gimple_build_switch (tree, tree, vec<tree> );
+gswitch *gimple_build_switch (tree, tree, const vec<tree> &);
 geh_dispatch *gimple_build_eh_dispatch (int);
 gdebug *gimple_build_debug_bind (tree, tree, gimple * CXX_MEM_STAT_INFO);
 gdebug *gimple_build_debug_source_bind (tree, tree, gimple * CXX_MEM_STAT_INFO);
@@ -1626,8 +1626,8 @@ extern bool nonbarrier_call_p (gimple *);
 extern bool infer_nonnull_range (gimple *, tree);
 extern bool infer_nonnull_range_by_dereference (gimple *, tree);
 extern bool infer_nonnull_range_by_attribute (gimple *, tree);
-extern void sort_case_labels (vec<tree>);
-extern void preprocess_case_label_vec_for_gimple (vec<tree>, tree, tree *);
+extern void sort_case_labels (vec<tree> &);
+extern void preprocess_case_label_vec_for_gimple (vec<tree> &, tree, tree *);
 extern void gimple_seq_set_location (gimple_seq, location_t);
 extern void gimple_seq_discard (gimple_seq);
 extern void maybe_remove_unused_call_args (struct function *, gimple *);
diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 9c88765d1fb..a166b706b8a 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -891,7 +891,7 @@ static void move_block_after_check (rtx_insn *);
 static void move_succs (vec<edge, va_gc> **, basic_block);
 static void sched_remove_insn (rtx_insn *);
 static void clear_priorities (rtx_insn *, rtx_vec_t *);
-static void calc_priorities (rtx_vec_t);
+static void calc_priorities (const rtx_vec_t &);
 static void add_jump_dependencies (rtx_insn *, rtx_insn *);
 
 #endif /* INSN_SCHEDULING */
@@ -7375,10 +7375,10 @@ haifa_sched_init (void)
     basic_block bb;
     FOR_EACH_BB_FN (bb, cfun)
       bbs.quick_push (bb);
-    sched_init_luids (bbs);
+    sched_init_luids (bbs.to_vec ());
     sched_deps_init (true);
     sched_extend_target ();
-    haifa_init_h_i_d (bbs);
+    haifa_init_h_i_d (bbs.to_vec ());
   }
 
   sched_init_only_bb = haifa_init_only_bb;
@@ -8923,7 +8923,7 @@ clear_priorities (rtx_insn *insn, rtx_vec_t *roots_ptr)
    changed.  ROOTS is a vector of instructions whose priority computation will
    trigger initialization of all cleared priorities.  */
 static void
-calc_priorities (rtx_vec_t roots)
+calc_priorities (const rtx_vec_t &roots)
 {
   int i;
   rtx_insn *insn;
@@ -8988,7 +8988,7 @@ sched_init_insn_luid (rtx_insn *insn)
    The hook common_sched_info->luid_for_non_insn () is used to determine
    if notes, labels, etc. need luids.  */
 void
-sched_init_luids (bb_vec_t bbs)
+sched_init_luids (const bb_vec_t &bbs)
 {
   int i;
   basic_block bb;
@@ -9062,7 +9062,7 @@ init_h_i_d (rtx_insn *insn)
 
 /* Initialize haifa_insn_data for BBS.  */
 void
-haifa_init_h_i_d (bb_vec_t bbs)
+haifa_init_h_i_d (const bb_vec_t &bbs)
 {
   int i;
   basic_block bb;
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 3d28a6e8640..511ec564846 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -499,10 +499,10 @@ public:
      get reallocated, the member vectors and the underlying auto_vecs would get
      out of sync.  */
   ipa_call_arg_values (ipa_auto_call_arg_values *aavals)
-    : m_known_vals (aavals->m_known_vals),
-      m_known_contexts (aavals->m_known_contexts),
-      m_known_aggs (aavals->m_known_aggs),
-      m_known_value_ranges (aavals->m_known_value_ranges)
+    : m_known_vals (aavals->m_known_vals.to_vec ()),
+      m_known_contexts (aavals->m_known_contexts.to_vec ()),
+      m_known_aggs (aavals->m_known_aggs.to_vec ()),
+      m_known_value_ranges (aavals->m_known_value_ranges.to_vec ())
   {}
 
   /* If m_known_vals (vector of known "scalar" values) is sufficiantly long,
diff --git a/gcc/ira-build.c b/gcc/ira-build.c
index 4031ce18287..42120656366 100644
--- a/gcc/ira-build.c
+++ b/gcc/ira-build.c
@@ -1672,7 +1672,7 @@ finish_cost_vectors (void)
 
 static vec<ira_loop_tree_node_t>
 ira_loop_tree_body_rev_postorder (ira_loop_tree_node_t loop_node ATTRIBUTE_UNUSED,
-				  vec<ira_loop_tree_node_t> loop_preorder)
+				  const vec<ira_loop_tree_node_t> &loop_preorder)
 {
   vec<ira_loop_tree_node_t> topsort_nodes = vNULL;
   unsigned int n_loop_preorder;
diff --git a/gcc/read-rtl.c b/gcc/read-rtl.c
index 925402877ec..e9eb1049e37 100644
--- a/gcc/read-rtl.c
+++ b/gcc/read-rtl.c
@@ -937,7 +937,7 @@ apply_iterators (rtx original, vec<rtx> *queue)
 	}
 
       if (oname)
-	add_overload_instance (oname, iterators, x);
+	add_overload_instance (oname, iterators.to_vec (), x);
 
       /* Add the new rtx to the end of the queue.  */
       queue->safe_push (x);
diff --git a/gcc/vec.h b/gcc/vec.h
index 30ef9a69473..6c58395ee69 100644
--- a/gcc/vec.h
+++ b/gcc/vec.h
@@ -547,11 +547,7 @@ vec_copy_construct (T *dst, const T *src, unsigned n)
    a vec instance, you can assign it the value vNULL.  This isn't
    needed for file-scope and function-local static vectors, which
    are zero-initialized by default.  */
-struct vnull
-{
-  template <typename T, typename A, typename L>
-  CONSTEXPR operator vec<T, A, L> () const { return vec<T, A, L>(); }
-};
+struct vnull { };
 extern vnull vNULL;
 
 
@@ -1431,10 +1427,25 @@ gt_pch_nx (vec<T, A, vl_embed> *v, gt_pointer_operator op, void *cookie)
    As long as we use C++03, we cannot have constructors nor
    destructors in classes that are stored in unions.  */
 
+template<typename T, size_t N = 0>
+class auto_vec;
+
 template<typename T>
 struct vec<T, va_heap, vl_ptr>
 {
 public:
+  vec () = default;
+  vec (const vec &) = default;
+  vec (vnull): m_vec () { }
+
+  /* Prevent implicit conversion from auto_vec.  Use auto_vec::to_vec()
+     instead.  */
+  template <size_t N>
+  vec (auto_vec<T, N> &) = delete;
+
+  template <size_t N>
+  void operator= (auto_vec<T, N> &) = delete;
+
   /* Memory allocation and deallocation for the embedded vector.
      Needed because we cannot have proper ctors/dtors defined.  */
   void create (unsigned nelems CXX_MEM_STAT_INFO);
@@ -1522,7 +1533,7 @@ public:
    want to ask for internal storage for vectors on the stack because if the
    size of the vector is larger than the internal storage that space is wasted.
    */
-template<typename T, size_t N = 0>
+template<typename T, size_t N /* = 0 */>
 class auto_vec : public vec<T, va_heap>
 {
 public:
@@ -1549,6 +1560,13 @@ public:
     this->release ();
   }
 
+  /* Explicitly convert to the base class.  There is no conversion
+     from a const auto_vec because a copy of the returned vec can
+     be used to modify *THIS.  */
+  vec<T, va_heap> to_vec () {
+    return *static_cast<vec<T, va_heap> *>(this);
+  }
+
 private:
   vec<T, va_heap, vl_embed> m_auto;
   T m_data[MAX (N - 1, 1)];
@@ -1602,6 +1620,13 @@ public:
       return *this;
     }
 
+  /* Explicitly convert to the base class.  There is no conversion
+     from a const auto_vec because a copy of the returned vec can
+     be used to modify *THIS.  */
+  vec<T, va_heap> to_vec () {
+    return *static_cast<vec<T, va_heap> *>(this);
+  }
+
   // You probably don't want to copy a vector, so these are deleted to prevent
   // unintentional use.  If you really need a copy of the vectors contents you
   // can use copy ().