Re: [RFA][PATCH] 10/n Various class definition cleanups

[Richard Biener <richard dot guenther at gmail dot com>]

On Sat, Nov 18, 2017 at 9:39 AM, Jeff Law <law@redhat.com> wrote:
>
> A batch of random cleanups in the various vrp related bits.
>
> The goal here is to start banging some of the classes into shape.
> Included in this patch:
>
>
> For evrp_folder:
>   Accepts vr_values in its ctor to attach.
>   Uses DISABLE_COPY_AND_ASSIGN
>
> For evrp_dom_walker:
>   Has the evrp_folder attached to it to avoid continually
>   recreating an evrp_folder instance.
>   Delegators removed, created free value_range_constant_singleton
>   along the way to help remove one of them.
>
> For evrp_range_analyzer:
>   The attached vr_values member is now private.
>   3 delegators are kept and made part of the public API
>   The remaining half-dozen or so delegators are gone
>
> For vr_values:
>   Privatizes several data members
>   Privatizes many methods
>
>
> I'm still not at all happy with management of the attached vr_values
> class instance.  I was going to use our unique_ptr, but without examples
> my attempts failed.  I'll have to give it another whirl when I'm fresher.
>
> Bootstrapped and regression tested on x86_64.
>
> OK for the trunk once the vr_values attachment code is cleaned up and it
> goes through another round of testing?

Ok.

Thanks,
Richard.

> jeff
>
>
>
>
>
>         * gimple-ssa-evrp-analyze.c (evrp_range_analyzer::evrp_range_analyzer)
>         Initialize vr_values.
>         (evrp_range_analyzer::try_find_new_range): Call methods attached to
>         vr_values via vr_values class instance rather than delegators.
>         (evrp_range_analyzer::record_ranges_from_phis): Likewise.
>         (evrp_range_analyzer::record_ranges_from_stmt): Likewise.
>         (evrp_range_analyzer::push_value_range): Likewise.
>         (evrp_range_analyzer::pop_value_range): Likewise.
>         * gimple-ssa-evrp-analyze.h (class evrp_range_analyzer): Remove
>         most delegators.  Those remaining are exposed as public interfaces.
>         Make vr_values a pointer.
>         (evrp_range_analyzer::~evrp_range_analyzer): Conditionally
>         delete the attached vr_values.
>         (evrp_range_analyzer::get_vr_value): New method.
>         * gimple-ssa-evrp.c (class evrp_folder): Use DISABLE_COPY_AND_ASSIGN.
>         (evrp_folder::evrp_folder): New ctor to initialize vr_values.
>         (class evrp_dom_walker): Attach evrp_folder class, initialize
>         it in the ctor.  Remove temporary delegators.
>         (evrp_dom_walker::before_dom_children): Call methods in attached
>         evrp_range_analyzer class via class instance pointer.  Use
>         free value_range_constant_singleton to remove need for
>         op_with_constant_singleton_value delegator method.  Do not
>         create a vrp_prop class instance for every call!  Narrow
>         scope of a couple variables.
>         (evrp_dom_walker::cleanup): Call methods in attached
>         evrp_range_analyzer class via class instance pointer.
>         * vr-values.h (class vr_values): Privatize many methods and
>         data members.
>
>
> diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
> index 9e581834d08..c3877791a5e 100644
> --- a/gcc/gimple-ssa-evrp-analyze.c
> +++ b/gcc/gimple-ssa-evrp-analyze.c
> @@ -53,6 +53,7 @@ evrp_range_analyzer::evrp_range_analyzer () : stack (10)
>        FOR_EACH_EDGE (e, ei, bb->preds)
>          e->flags |= EDGE_EXECUTABLE;
>      }
> +  vr_values = new class vr_values;
>  }
>
>  void
> @@ -73,8 +74,8 @@ evrp_range_analyzer::try_find_new_range (tree name,
>    value_range *old_vr = get_value_range (name);
>
>    /* Discover VR when condition is true.  */
> -  extract_range_for_var_from_comparison_expr (name, code, op,
> -                                             limit, &vr);
> +  vr_values->extract_range_for_var_from_comparison_expr (name, code, op,
> +                                                        limit, &vr);
>    /* If we found any usable VR, set the VR to ssa_name and create a
>       PUSH old value in the stack with the old VR.  */
>    if (vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE)
> @@ -83,7 +84,7 @@ evrp_range_analyzer::try_find_new_range (tree name,
>           && vrp_operand_equal_p (old_vr->min, vr.min)
>           && vrp_operand_equal_p (old_vr->max, vr.max))
>         return NULL;
> -      value_range *new_vr = vr_values.vrp_value_range_pool.allocate ();
> +      value_range *new_vr = vr_values->vrp_value_range_pool.allocate ();
>        *new_vr = vr;
>        return new_vr;
>      }
> @@ -167,7 +168,7 @@ evrp_range_analyzer::record_ranges_from_phis (basic_block bb)
>        value_range vr_result = VR_INITIALIZER;
>        bool interesting = stmt_interesting_for_vrp (phi);
>        if (!has_unvisited_preds && interesting)
> -       extract_range_from_phi_node (phi, &vr_result);
> +       vr_values->extract_range_from_phi_node (phi, &vr_result);
>        else
>         {
>           set_value_range_to_varying (&vr_result);
> @@ -179,9 +180,9 @@ evrp_range_analyzer::record_ranges_from_phis (basic_block bb)
>           if (interesting
>               && (l = loop_containing_stmt (phi))
>               && l->header == gimple_bb (phi))
> -         adjust_range_with_scev (&vr_result, l, phi, lhs);
> +         vr_values->adjust_range_with_scev (&vr_result, l, phi, lhs);
>         }
> -      update_value_range (lhs, &vr_result);
> +      vr_values->update_value_range (lhs, &vr_result);
>
>        /* Set the SSA with the value range.  */
>        if (INTEGRAL_TYPE_P (TREE_TYPE (lhs)))
> @@ -216,11 +217,11 @@ evrp_range_analyzer::record_ranges_from_stmt (gimple *stmt)
>      {
>        edge taken_edge;
>        value_range vr = VR_INITIALIZER;
> -      extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
> +      vr_values->extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
>        if (output
>           && (vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE))
>         {
> -         update_value_range (output, &vr);
> +         vr_values->update_value_range (output, &vr);
>
>           /* Set the SSA with the value range.  */
>           if (INTEGRAL_TYPE_P (TREE_TYPE (output)))
> @@ -240,10 +241,10 @@ evrp_range_analyzer::record_ranges_from_stmt (gimple *stmt)
>             set_ptr_nonnull (output);
>         }
>        else
> -       set_defs_to_varying (stmt);
> +       vr_values->set_defs_to_varying (stmt);
>      }
>    else
> -    set_defs_to_varying (stmt);
> +    vr_values->set_defs_to_varying (stmt);
>
>    /* See if we can derive a range for any of STMT's operands.  */
>    tree op;
> @@ -319,7 +320,7 @@ evrp_range_analyzer::push_value_range (tree var, value_range *vr)
>        fprintf (dump_file, "\n");
>      }
>    stack.safe_push (std::make_pair (var, get_value_range (var)));
> -  set_vr_value (var, vr);
> +  vr_values->set_vr_value (var, vr);
>  }
>
>  /* Pop the Value Range from the vrp_stack and update VAR with it.  */
> @@ -337,7 +338,7 @@ evrp_range_analyzer::pop_value_range (tree var)
>        dump_value_range (dump_file, vr);
>        fprintf (dump_file, "\n");
>      }
> -  set_vr_value (var, vr);
> +  vr_values->set_vr_value (var, vr);
>    stack.pop ();
>    return vr;
>  }
> diff --git a/gcc/gimple-ssa-evrp-analyze.h b/gcc/gimple-ssa-evrp-analyze.h
> index 2e6c609c45b..b60bba848cc 100644
> --- a/gcc/gimple-ssa-evrp-analyze.h
> +++ b/gcc/gimple-ssa-evrp-analyze.h
> @@ -24,16 +24,46 @@ class evrp_range_analyzer
>  {
>   public:
>    evrp_range_analyzer (void);
> -  ~evrp_range_analyzer (void) { stack.release (); }
> +  ~evrp_range_analyzer (void)
> +  {
> +    if (vr_values)
> +      delete vr_values;
> +    stack.release ();
> +  }
>
>    void enter (basic_block);
>    void leave (basic_block);
>    void record_ranges_from_stmt (gimple *);
>
> -  class vr_values vr_values;
> +  /* Main interface to retrieve range information.  */
> +  value_range *get_value_range (const_tree op)
> +    { return vr_values->get_value_range (op); }
> +
> +  /* Dump all the current value ranges.  This is primarily
> +     a debugging interface.  */
> +  void dump_all_value_ranges (FILE *fp)
> +    { vr_values->dump_all_value_ranges (fp); }
> +
> +  /* A bit of a wart.  This should ideally go away.  */
> +  void vrp_visit_cond_stmt (gcond *cond, edge *e)
> +    { return vr_values->vrp_visit_cond_stmt (cond, e); }
> +
> +  /* Get the underlying vr_values class instance.  If TRANSFER is
> +     true, then we are transferring ownership.  Else we keep ownership.
> +
> +     This should be converted to a unique_ptr.  */
> +  class vr_values *get_vr_values (bool transfer)
> +    {
> +      class vr_values *x = vr_values;
> +      if (transfer)
> +       vr_values = NULL;
> +      return x;
> +    }
>
>   private:
>    DISABLE_COPY_AND_ASSIGN (evrp_range_analyzer);
> +  class vr_values *vr_values;
> +
>    void push_value_range (tree var, value_range *vr);
>    value_range *pop_value_range (tree var);
>    value_range *try_find_new_range (tree, tree op, tree_code code, tree limit);
> @@ -42,30 +72,6 @@ class evrp_range_analyzer
>
>    /* STACK holds the old VR.  */
>    auto_vec<std::pair <tree, value_range*> > stack;
> -
> -  /* Temporary delegators.  */
> -  value_range *get_value_range (const_tree op)
> -    { return vr_values.get_value_range (op); }
> -  bool update_value_range (const_tree op, value_range *vr)
> -    { return vr_values.update_value_range (op, vr); }
> -  void extract_range_from_phi_node (gphi *phi, value_range *vr)
> -    { vr_values.extract_range_from_phi_node (phi, vr); }
> -  void adjust_range_with_scev (value_range *vr, struct loop *loop,
> -                               gimple *stmt, tree var)
> -    { vr_values.adjust_range_with_scev (vr, loop, stmt, var); }
> -  void extract_range_from_stmt (gimple *stmt, edge *taken_edge_p,
> -                                tree *output_p, value_range *vr)
> -    { vr_values.extract_range_from_stmt (stmt, taken_edge_p, output_p, vr); }
> -  void set_defs_to_varying (gimple *stmt)
> -    { return vr_values.set_defs_to_varying (stmt); }
> -  void set_vr_value (tree name, value_range *vr)
> -    { vr_values.set_vr_value (name, vr); }
> -  void extract_range_for_var_from_comparison_expr (tree var,
> -                                                  enum tree_code cond_code,
> -                                                  tree op, tree limit,
> -                                                  value_range *vr_p)
> -    { vr_values.extract_range_for_var_from_comparison_expr (var, cond_code,
> -                                                           op, limit, vr_p); }
>  };
>
>  #endif /* GCC_GIMPLE_SSA_EVRP_ANALYZE_H */
> diff --git a/gcc/gimple-ssa-evrp.c b/gcc/gimple-ssa-evrp.c
> index 27a983dd9ae..a554cf9e834 100644
> --- a/gcc/gimple-ssa-evrp.c
> +++ b/gcc/gimple-ssa-evrp.c
> @@ -46,8 +46,11 @@ class evrp_folder : public substitute_and_fold_engine
>  {
>   public:
>    tree get_value (tree) FINAL OVERRIDE;
> -
> +  evrp_folder (class vr_values *vr_values_) : vr_values (vr_values_) { }
>    class vr_values *vr_values;
> +
> + private:
> +  DISABLE_COPY_AND_ASSIGN (evrp_folder);
>  };
>
>  tree
> @@ -63,7 +66,9 @@ evrp_folder::get_value (tree op)
>  class evrp_dom_walker : public dom_walker
>  {
>  public:
> -  evrp_dom_walker () : dom_walker (CDI_DOMINATORS)
> +  evrp_dom_walker ()
> +    : dom_walker (CDI_DOMINATORS),
> +      evrp_folder (evrp_range_analyzer.get_vr_values (false))
>      {
>        need_eh_cleanup = BITMAP_ALLOC (NULL);
>      }
> @@ -82,14 +87,7 @@ public:
>    auto_vec<gimple *> stmts_to_remove;
>
>    class evrp_range_analyzer evrp_range_analyzer;
> -
> -  /* Temporary delegators.  */
> -  value_range *get_value_range (const_tree op)
> -    { return evrp_range_analyzer.vr_values.get_value_range (op); }
> -  tree op_with_constant_singleton_value_range (tree op)
> -    { return evrp_range_analyzer.vr_values.op_with_constant_singleton_value_range (op); }
> -  void vrp_visit_cond_stmt (gcond *cond, edge *e)
> -    { evrp_range_analyzer.vr_values.vrp_visit_cond_stmt (cond, e); }
> +  class evrp_folder evrp_folder;
>  };
>
>  edge
> @@ -108,8 +106,9 @@ evrp_dom_walker::before_dom_children (basic_block bb)
>        if (virtual_operand_p (lhs))
>         continue;
>
> +      value_range *vr = evrp_range_analyzer.get_value_range (lhs);
>        /* Mark PHIs whose lhs we fully propagate for removal.  */
> -      tree val = op_with_constant_singleton_value_range (lhs);
> +      tree val = value_range_constant_singleton (vr);
>        if (val && may_propagate_copy (lhs, val))
>         {
>           stmts_to_remove.safe_push (phi);
> @@ -139,7 +138,7 @@ evrp_dom_walker::before_dom_children (basic_block bb)
>
>        if (gcond *cond = dyn_cast <gcond *> (stmt))
>         {
> -         vrp_visit_cond_stmt (cond, &taken_edge);
> +         evrp_range_analyzer.vrp_visit_cond_stmt (cond, &taken_edge);
>           if (taken_edge)
>             {
>               if (taken_edge->flags & EDGE_TRUE_VALUE)
> @@ -153,16 +152,15 @@ evrp_dom_walker::before_dom_children (basic_block bb)
>         }
>        else if (stmt_interesting_for_vrp (stmt))
>         {
> -         value_range vr = VR_INITIALIZER;
>           output = get_output_for_vrp (stmt);
>           if (output)
>             {
>               tree val;
> -             vr = *get_value_range (output);
> +             value_range *vr = evrp_range_analyzer.get_value_range (output);
>
>               /* Mark stmts whose output we fully propagate for removal.  */
> -             if ((vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE)
> -                 && (val = op_with_constant_singleton_value_range (output))
> +             if ((vr->type == VR_RANGE || vr->type == VR_ANTI_RANGE)
> +                 && (val = value_range_constant_singleton (vr))
>                   && may_propagate_copy (output, val)
>                   && !stmt_could_throw_p (stmt)
>                   && !gimple_has_side_effects (stmt))
> @@ -174,8 +172,6 @@ evrp_dom_walker::before_dom_children (basic_block bb)
>         }
>
>        /* Try folding stmts with the VR discovered.  */
> -      class evrp_folder evrp_folder;
> -      evrp_folder.vr_values = &evrp_range_analyzer.vr_values;
>        bool did_replace = evrp_folder.replace_uses_in (stmt);
>        if (fold_stmt (&gsi, follow_single_use_edges)
>           || did_replace)
> @@ -222,7 +218,8 @@ evrp_dom_walker::before_dom_children (basic_block bb)
>           if (TREE_CODE (arg) != SSA_NAME
>               || virtual_operand_p (arg))
>             continue;
> -         tree val = op_with_constant_singleton_value_range (arg);
> +         value_range *vr = evrp_range_analyzer.get_value_range (arg);
> +         tree val = value_range_constant_singleton (vr);
>           if (val && may_propagate_copy (arg, val))
>             propagate_value (use_p, val);
>         }
> @@ -245,7 +242,7 @@ evrp_dom_walker::cleanup (void)
>    if (dump_file)
>      {
>        fprintf (dump_file, "\nValue ranges after Early VRP:\n\n");
> -      evrp_range_analyzer.vr_values.dump_all_value_ranges (dump_file);
> +      evrp_range_analyzer.dump_all_value_ranges (dump_file);
>        fprintf (dump_file, "\n");
>      }
>
> diff --git a/gcc/vr-values.h b/gcc/vr-values.h
> index 24f013a9402..9eeebedfaed 100644
> --- a/gcc/vr-values.h
> +++ b/gcc/vr-values.h
> @@ -40,80 +40,81 @@ class vr_values
>    vr_values (void);
>    ~vr_values (void);
>
> -  /* Value range array.  After propagation, VR_VALUE[I] holds the range
> -     of values that SSA name N_I may take.  */
> -  unsigned int num_vr_values;
> -  value_range **vr_value;
> -  bool values_propagated;
> +  value_range *get_value_range (const_tree);
>
> -  /* For a PHI node which sets SSA name N_I, VR_COUNTS[I] holds the
> -     number of executable edges we saw the last time we visited the
> -     node.  */
> -  int *vr_phi_edge_counts;
> +  void set_vr_value (tree, value_range *);
> +  void set_defs_to_varying (gimple *);
> +  bool update_value_range (const_tree, value_range *);
> +  tree op_with_constant_singleton_value_range (tree);
> +  void adjust_range_with_scev (value_range *, struct loop *, gimple *, tree);
> +  tree vrp_evaluate_conditional (tree_code, tree, tree, gimple *);
> +  void dump_all_value_ranges (FILE *);
> +
> +  void extract_range_for_var_from_comparison_expr (tree, enum tree_code,
> +                                                  tree, tree, value_range *);
> +  void extract_range_from_phi_node (gphi *, value_range *);
> +  void extract_range_basic (value_range *, gimple *);
> +  void extract_range_from_assignment (value_range *, gassign *);
> +  void extract_range_from_stmt (gimple *, edge *, tree *, value_range *);
> +
> +  void vrp_visit_cond_stmt (gcond *, edge *);
> +
> +  void simplify_cond_using_ranges_2 (gcond *);
> +  bool simplify_stmt_using_ranges (gimple_stmt_iterator *);
> +
> +  /* This probably belongs in the lattice rather than in here.  */
> +  bool values_propagated;
>
>    /* Allocation pools for tree-vrp allocations.  */
>    object_allocator<value_range> vrp_value_range_pool;
> -  bitmap_obstack vrp_equiv_obstack;
>
> -  value_range *get_value_range (const_tree);
> -  void set_vr_value (tree, value_range *);
> -
> -  void set_defs_to_varying (gimple *);
> -  bool update_value_range (const_tree, value_range *);
> + private:
> +  bitmap_obstack vrp_equiv_obstack;
>    void add_equivalence (bitmap *, const_tree);
>    bool vrp_stmt_computes_nonzero (gimple *);
> -  tree op_with_constant_singleton_value_range (tree);
>    bool op_with_boolean_value_range_p (tree);
>    bool check_for_binary_op_overflow (enum tree_code, tree, tree, tree, bool *);
> -  void adjust_range_with_scev (value_range *, struct loop *, gimple *, tree);
>    value_range get_vr_for_comparison (int);
>    tree compare_name_with_value (enum tree_code, tree, tree, bool *, bool);
>    tree compare_names (enum tree_code, tree, tree, bool *);
>    bool two_valued_val_range_p (tree, tree *, tree *);
> -
>    tree vrp_evaluate_conditional_warnv_with_ops_using_ranges (enum tree_code,
>                                                              tree, tree,
>                                                              bool *);
>    tree vrp_evaluate_conditional_warnv_with_ops (enum tree_code,
>                                                 tree, tree, bool,
>                                                 bool *, bool *);
> -  tree vrp_evaluate_conditional (tree_code, tree, tree, gimple *);
> -
> -
> -  void dump_all_value_ranges (FILE *);
> -
> -  void extract_range_for_var_from_comparison_expr (tree, enum tree_code,
> -                                                  tree, tree, value_range *);
>    void extract_range_from_assert (value_range *, tree);
>    void extract_range_from_ssa_name (value_range *, tree);
>    void extract_range_from_binary_expr (value_range *, enum tree_code,
>                                        tree, tree, tree);
>    void extract_range_from_unary_expr (value_range *, enum tree_code,
>                                       tree, tree);
> -  void extract_range_from_phi_node (gphi *, value_range *);
>    void extract_range_from_cond_expr (value_range *, gassign *);
> -  void extract_range_basic (value_range *, gimple *);
> -  void extract_range_from_assignment (value_range *, gassign *);
> -  void extract_range_from_stmt (gimple *, edge *, tree *, value_range *);
>    void extract_range_from_comparison (value_range *, enum tree_code,
>                                       tree, tree, tree);
> -
>    void vrp_visit_assignment_or_call (gimple*, tree *, value_range *);
>    void vrp_visit_switch_stmt (gswitch *, edge *);
> -  void vrp_visit_cond_stmt (gcond *, edge *);
> -
>    bool simplify_truth_ops_using_ranges (gimple_stmt_iterator *, gimple *);
>    bool simplify_div_or_mod_using_ranges (gimple_stmt_iterator *, gimple *);
>    bool simplify_abs_using_ranges (gimple_stmt_iterator *, gimple *);
>    bool simplify_bit_ops_using_ranges (gimple_stmt_iterator *, gimple *);
>    bool simplify_min_or_max_using_ranges (gimple_stmt_iterator *, gimple *);
>    bool simplify_cond_using_ranges_1 (gcond *);
> -  void simplify_cond_using_ranges_2 (gcond *);
>    bool simplify_switch_using_ranges (gswitch *);
>    bool simplify_float_conversion_using_ranges (gimple_stmt_iterator *,
>                                                gimple *);
>    bool simplify_internal_call_using_ranges (gimple_stmt_iterator *, gimple *);
> -  bool simplify_stmt_using_ranges (gimple_stmt_iterator *);
> +
> +  /* Value range array.  After propagation, VR_VALUE[I] holds the range
> +     of values that SSA name N_I may take.  */
> +  unsigned int num_vr_values;
> +  value_range **vr_value;
> +
> +  /* For a PHI node which sets SSA name N_I, VR_COUNTS[I] holds the
> +     number of executable edges we saw the last time we visited the
> +     node.  */
> +  int *vr_phi_edge_counts;
>  };
>
>  #define VR_INITIALIZER { VR_UNDEFINED, NULL_TREE, NULL_TREE, NULL }
>