Convert ipcp_vr_lattice to type agnostic framework.

This converts the lattice to store ranges in Value_Range instead of
value_range (*) to make it type agnostic, and adjust all users
accordingly.

I think it is a good example on converting from static ranges to more
general, type agnostic ones.

I've been careful to make sure Value_Range never ends up on GC, since
it contains an int_range_max and can expand on-demand onto the heap.
Longer term storage for ranges should be done with vrange_storage, as
per the previous patch ("Provide an API for ipa_vr").

(*) I do know the Value_Range naming versus value_range is quite
annoying, but it was a judgement call last release for the eventual
migration to having "value_range" be a type agnostic range object.  We
will ultimately rename Value_Range to value_range.

OK for trunk?

gcc/ChangeLog:

	* ipa-cp.cc (ipcp_vr_lattice::init): Take type argument.
	(ipcp_vr_lattice::print): Call dump method.
	(ipcp_vr_lattice::meet_with): Adjust for m_vr being a
	Value_Range.
	(ipcp_vr_lattice::meet_with_1): Make argument a reference.
	(ipcp_vr_lattice::set_to_bottom): Add type argument.
	(set_all_contains_variable): Same.
	(initialize_node_lattices): Pass type when appropriate.
	(ipa_vr_operation_and_type_effects): Make type agnostic.
	(ipa_value_range_from_jfunc): Same.
	(propagate_vr_across_jump_function): Same.
	(propagate_constants_across_call): Same.
	* ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same.
	(evaluate_properties_for_edge): Same.
	* ipa-prop.cc (ipcp_update_vr): Same.
	* ipa-prop.h (ipa_value_range_from_jfunc): Same.
	(ipa_range_set_and_normalize): Same.
---
 gcc/ipa-cp.cc        | 159 +++++++++++++++++++++++--------------------
 gcc/ipa-fnsummary.cc |  16 ++---
 gcc/ipa-prop.cc      |   2 +-
 gcc/ipa-prop.h       |  19 ++----
 4 files changed, 101 insertions(+), 95 deletions(-)

On 5/17/23 16:30, Aldy Hernandez wrote:
> This converts the lattice to store ranges in Value_Range instead of
> value_range (*) to make it type agnostic, and adjust all users
> accordingly.
> 
> I think it is a good example on converting from static ranges to more
> general, type agnostic ones.
> 
> I've been careful to make sure Value_Range never ends up on GC, since
> it contains an int_range_max and can expand on-demand onto the heap.
> Longer term storage for ranges should be done with vrange_storage, as
> per the previous patch ("Provide an API for ipa_vr").
> 
> (*) I do know the Value_Range naming versus value_range is quite
> annoying, but it was a judgement call last release for the eventual
> migration to having "value_range" be a type agnostic range object.  We
> will ultimately rename Value_Range to value_range.

I forgot to mention.  This doesn't make IPA be type agnostic per se, 
just the range usage throughout.  The IPA code is still guarded by stuff 
like:

   if (!param_type
       || (!INTEGRAL_TYPE_P (param_type)
	  && !POINTER_TYPE_P (param_type)))
     return dest_lat->set_to_bottom (param_type);

It is up to the maintainers to adjust their passes, as I'm liable to 
break everything in the process ;-).

The above should probably become:

    if (!param_type || !Value_Range::supports_type_p (param_type))
...

This is the canonical way of querying whether a type is supported by 
Value_Range, the ranger temporary that can handle each supported type, 
and thus the ranger.  This is documented here:

// To query what types ranger and the entire ecosystem can support,
// use Value_Range::supports_type_p(tree type).  This is a static
// method available independently of any vrange object.
//
// To query what a given vrange variant can support, use:
//    irange::supports_p ()
//    frange::supports_p ()
//    etc

However, with the changes I have posted so far, ranges throughout have a 
much finer granularity and are no longer limited to the 2-sub-ranges in 
a value_range.  If you look at IPA dumps now, you'll see the ranges are 
much more refined and are streamed for LTO accordingly.  This is an 
improvement in and of itself.

Aldy

I've adjusted the patch with some minor cleanups that came up when I
implemented the rest of the IPA revamp.

Rested.  OK?

On Wed, May 17, 2023 at 4:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> This converts the lattice to store ranges in Value_Range instead of
> value_range (*) to make it type agnostic, and adjust all users
> accordingly.
>
> I think it is a good example on converting from static ranges to more
> general, type agnostic ones.
>
> I've been careful to make sure Value_Range never ends up on GC, since
> it contains an int_range_max and can expand on-demand onto the heap.
> Longer term storage for ranges should be done with vrange_storage, as
> per the previous patch ("Provide an API for ipa_vr").
>
> (*) I do know the Value_Range naming versus value_range is quite
> annoying, but it was a judgement call last release for the eventual
> migration to having "value_range" be a type agnostic range object.  We
> will ultimately rename Value_Range to value_range.
>
> OK for trunk?
>
> gcc/ChangeLog:
>
>         * ipa-cp.cc (ipcp_vr_lattice::init): Take type argument.
>         (ipcp_vr_lattice::print): Call dump method.
>         (ipcp_vr_lattice::meet_with): Adjust for m_vr being a
>         Value_Range.
>         (ipcp_vr_lattice::meet_with_1): Make argument a reference.
>         (ipcp_vr_lattice::set_to_bottom): Add type argument.
>         (set_all_contains_variable): Same.
>         (initialize_node_lattices): Pass type when appropriate.
>         (ipa_vr_operation_and_type_effects): Make type agnostic.
>         (ipa_value_range_from_jfunc): Same.
>         (propagate_vr_across_jump_function): Same.
>         (propagate_constants_across_call): Same.
>         * ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same.
>         (evaluate_properties_for_edge): Same.
>         * ipa-prop.cc (ipcp_update_vr): Same.
>         * ipa-prop.h (ipa_value_range_from_jfunc): Same.
>         (ipa_range_set_and_normalize): Same.
> ---
>  gcc/ipa-cp.cc        | 159 +++++++++++++++++++++++--------------------
>  gcc/ipa-fnsummary.cc |  16 ++---
>  gcc/ipa-prop.cc      |   2 +-
>  gcc/ipa-prop.h       |  19 ++----
>  4 files changed, 101 insertions(+), 95 deletions(-)
>
> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
> index d4b9d4ac27e..bd5b1da17b2 100644
> --- a/gcc/ipa-cp.cc
> +++ b/gcc/ipa-cp.cc
> @@ -343,20 +343,29 @@ private:
>  class ipcp_vr_lattice
>  {
>  public:
> -  value_range m_vr;
> +  Value_Range m_vr;
>
>    inline bool bottom_p () const;
>    inline bool top_p () const;
> -  inline bool set_to_bottom ();
> -  bool meet_with (const value_range *p_vr);
> +  inline bool set_to_bottom (tree type);
> +  bool meet_with (const vrange &p_vr);
>    bool meet_with (const ipcp_vr_lattice &other);
> -  void init () { gcc_assert (m_vr.undefined_p ()); }
> +  void init (tree type);
>    void print (FILE * f);
>
>  private:
> -  bool meet_with_1 (const value_range *other_vr);
> +  bool meet_with_1 (const vrange &other_vr);
>  };
>
> +inline void
> +ipcp_vr_lattice::init (tree type)
> +{
> +  if (type)
> +    m_vr.set_type (type);
> +
> +  // Otherwise m_vr will default to unsupported_range.
> +}
> +
>  /* Structure containing lattices for a parameter itself and for pieces of
>     aggregates that are passed in the parameter or by a reference in a parameter
>     plus some other useful flags.  */
> @@ -585,7 +594,7 @@ ipcp_bits_lattice::print (FILE *f)
>  void
>  ipcp_vr_lattice::print (FILE * f)
>  {
> -  dump_value_range (f, &m_vr);
> +  m_vr.dump (f);
>  }
>
>  /* Print all ipcp_lattices of all functions to F.  */
> @@ -1016,14 +1025,14 @@ set_agg_lats_contain_variable (class ipcp_param_lattices *plats)
>  bool
>  ipcp_vr_lattice::meet_with (const ipcp_vr_lattice &other)
>  {
> -  return meet_with_1 (&other.m_vr);
> +  return meet_with_1 (other.m_vr);
>  }
>
>  /* Meet the current value of the lattice with value range described by VR
>     lattice.  */
>
>  bool
> -ipcp_vr_lattice::meet_with (const value_range *p_vr)
> +ipcp_vr_lattice::meet_with (const vrange &p_vr)
>  {
>    return meet_with_1 (p_vr);
>  }
> @@ -1032,23 +1041,23 @@ ipcp_vr_lattice::meet_with (const value_range *p_vr)
>     OTHER_VR lattice.  Return TRUE if anything changed.  */
>
>  bool
> -ipcp_vr_lattice::meet_with_1 (const value_range *other_vr)
> +ipcp_vr_lattice::meet_with_1 (const vrange &other_vr)
>  {
>    if (bottom_p ())
>      return false;
>
> -  if (other_vr->varying_p ())
> -    return set_to_bottom ();
> +  if (other_vr.varying_p ())
> +    return set_to_bottom (other_vr.type ());
>
>    bool res;
>    if (flag_checking)
>      {
> -      value_range save (m_vr);
> -      res = m_vr.union_ (*other_vr);
> +      Value_Range save (m_vr);
> +      res = m_vr.union_ (other_vr);
>        gcc_assert (res == (m_vr != save));
>      }
>    else
> -    res = m_vr.union_ (*other_vr);
> +    res = m_vr.union_ (other_vr);
>    return res;
>  }
>
> @@ -1073,16 +1082,11 @@ ipcp_vr_lattice::bottom_p () const
>     previously was in a different state.  */
>
>  bool
> -ipcp_vr_lattice::set_to_bottom ()
> +ipcp_vr_lattice::set_to_bottom (tree type)
>  {
>    if (m_vr.varying_p ())
>      return false;
> -  /* ?? We create all sorts of VARYING ranges for floats, structures,
> -     and other types which we cannot handle as ranges.  We should
> -     probably avoid handling them throughout the pass, but it's easier
> -     to create a sensible VARYING here and let the lattice
> -     propagate.  */
> -  m_vr.set_varying (integer_type_node);
> +  m_vr.set_varying (type);
>    return true;
>  }
>
> @@ -1518,14 +1522,14 @@ intersect_argaggs_with (vec<ipa_argagg_value> &elts,
>     return true is any of them has not been marked as such so far.  */
>
>  static inline bool
> -set_all_contains_variable (class ipcp_param_lattices *plats)
> +set_all_contains_variable (class ipcp_param_lattices *plats, tree type)
>  {
>    bool ret;
>    ret = plats->itself.set_contains_variable ();
>    ret |= plats->ctxlat.set_contains_variable ();
>    ret |= set_agg_lats_contain_variable (plats);
>    ret |= plats->bits_lattice.set_to_bottom ();
> -  ret |= plats->m_value_range.set_to_bottom ();
> +  ret |= plats->m_value_range.set_to_bottom (type);
>    return ret;
>  }
>
> @@ -1653,6 +1657,7 @@ initialize_node_lattices (struct cgraph_node *node)
>    for (i = 0; i < ipa_get_param_count (info); i++)
>      {
>        ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
> +      tree type = ipa_get_type (info, i);
>        if (disable
>           || !ipa_get_type (info, i)
>           || (pre_modified && (surviving_params.length () <= (unsigned) i
> @@ -1662,14 +1667,14 @@ initialize_node_lattices (struct cgraph_node *node)
>           plats->ctxlat.set_to_bottom ();
>           set_agg_lats_to_bottom (plats);
>           plats->bits_lattice.set_to_bottom ();
> -         plats->m_value_range.m_vr = value_range ();
> -         plats->m_value_range.set_to_bottom ();
> +         plats->m_value_range.init (type);
> +         plats->m_value_range.set_to_bottom (type);
>         }
>        else
>         {
> -         plats->m_value_range.init ();
> +         plats->m_value_range.init (type);
>           if (variable)
> -           set_all_contains_variable (plats);
> +           set_all_contains_variable (plats, type);
>         }
>      }
>
> @@ -1903,8 +1908,8 @@ ipa_context_from_jfunc (ipa_node_params *info, cgraph_edge *cs, int csidx,
>     the result is a range or an anti-range.  */
>
>  static bool
> -ipa_vr_operation_and_type_effects (value_range *dst_vr,
> -                                  value_range *src_vr,
> +ipa_vr_operation_and_type_effects (vrange &dst_vr,
> +                                  const vrange &src_vr,
>                                    enum tree_code operation,
>                                    tree dst_type, tree src_type)
>  {
> @@ -1912,29 +1917,33 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr,
>      return false;
>
>    range_op_handler handler (operation, dst_type);
> -  return (handler
> -         && handler.fold_range (*dst_vr, dst_type,
> -                                *src_vr, value_range (dst_type))
> -         && !dst_vr->varying_p ()
> -         && !dst_vr->undefined_p ());
> +  if (!handler)
> +    return false;
> +
> +  Value_Range varying (dst_type);
> +  varying.set_varying (dst_type);
> +
> +  return (handler.fold_range (dst_vr, dst_type, src_vr, varying)
> +         && !dst_vr.varying_p ()
> +         && !dst_vr.undefined_p ());
>  }
>
>  /* Determine value_range of JFUNC given that INFO describes the caller node or
>     the one it is inlined to, CS is the call graph edge corresponding to JFUNC
>     and PARM_TYPE of the parameter.  */
>
> -value_range
> -ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
> +void
> +ipa_value_range_from_jfunc (vrange &vr,
> +                           ipa_node_params *info, cgraph_edge *cs,
>                             ipa_jump_func *jfunc, tree parm_type)
>  {
> -  value_range vr;
>    if (jfunc->m_vr)
> -    ipa_vr_operation_and_type_effects (&vr,
> -                                      jfunc->m_vr,
> +    ipa_vr_operation_and_type_effects (vr,
> +                                      *jfunc->m_vr,
>                                        NOP_EXPR, parm_type,
>                                        jfunc->m_vr->type ());
>    if (vr.singleton_p ())
> -    return vr;
> +    return;
>    if (jfunc->type == IPA_JF_PASS_THROUGH)
>      {
>        int idx;
> @@ -1943,33 +1952,34 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>                                            ? cs->caller->inlined_to
>                                            : cs->caller);
>        if (!sum || !sum->m_vr)
> -       return vr;
> +       return;
>
>        idx = ipa_get_jf_pass_through_formal_id (jfunc);
>
>        if (!(*sum->m_vr)[idx].known_p ())
> -       return vr;
> +       return;
>        tree vr_type = ipa_get_type (info, idx);
> -      value_range srcvr;
> +      Value_Range srcvr (vr_type);
>        (*sum->m_vr)[idx].get_vrange (srcvr, vr_type);
>
>        enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>
>        if (TREE_CODE_CLASS (operation) == tcc_unary)
>         {
> -         value_range res;
> +         Value_Range res (vr_type);
>
> -         if (ipa_vr_operation_and_type_effects (&res,
> -                                                &srcvr,
> +         if (ipa_vr_operation_and_type_effects (res,
> +                                                srcvr,
>                                                  operation, parm_type,
>                                                  vr_type))
>             vr.intersect (res);
>         }
>        else
>         {
> -         value_range op_res, res;
> +         Value_Range op_res (vr_type);
> +         Value_Range res (vr_type);
>           tree op = ipa_get_jf_pass_through_operand (jfunc);
> -         value_range op_vr;
> +         Value_Range op_vr (vr_type);
>           range_op_handler handler (operation, vr_type);
>
>           ipa_range_set_and_normalize (op_vr, op);
> @@ -1979,14 +1989,13 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>               || !handler.fold_range (op_res, vr_type, srcvr, op_vr))
>             op_res.set_varying (vr_type);
>
> -         if (ipa_vr_operation_and_type_effects (&res,
> -                                                &op_res,
> +         if (ipa_vr_operation_and_type_effects (res,
> +                                                op_res,
>                                                  NOP_EXPR, parm_type,
>                                                  vr_type))
>             vr.intersect (res);
>         }
>      }
> -  return vr;
>  }
>
>  /* Determine whether ITEM, jump function for an aggregate part, evaluates to a
> @@ -2739,7 +2748,7 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>    if (!param_type
>        || (!INTEGRAL_TYPE_P (param_type)
>           && !POINTER_TYPE_P (param_type)))
> -    return dest_lat->set_to_bottom ();
> +    return dest_lat->set_to_bottom (param_type);
>
>    if (jfunc->type == IPA_JF_PASS_THROUGH)
>      {
> @@ -2751,12 +2760,12 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>        tree operand_type = ipa_get_type (caller_info, src_idx);
>
>        if (src_lats->m_value_range.bottom_p ())
> -       return dest_lat->set_to_bottom ();
> +       return dest_lat->set_to_bottom (operand_type);
>
> -      value_range vr;
> +      Value_Range vr (operand_type);
>        if (TREE_CODE_CLASS (operation) == tcc_unary)
> -       ipa_vr_operation_and_type_effects (&vr,
> -                                          &src_lats->m_value_range.m_vr,
> +       ipa_vr_operation_and_type_effects (vr,
> +                                          src_lats->m_value_range.m_vr,
>                                            operation, param_type,
>                                            operand_type);
>        /* A crude way to prevent unbounded number of value range updates
> @@ -2765,8 +2774,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>        else if (!ipa_edge_within_scc (cs))
>         {
>           tree op = ipa_get_jf_pass_through_operand (jfunc);
> -         value_range op_vr;
> -         value_range op_res,res;
> +         Value_Range op_vr (TREE_TYPE (op));
> +         Value_Range op_res (operand_type);
>           range_op_handler handler (operation, operand_type);
>
>           ipa_range_set_and_normalize (op_vr, op);
> @@ -2777,8 +2786,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>                                       src_lats->m_value_range.m_vr, op_vr))
>             op_res.set_varying (operand_type);
>
> -         ipa_vr_operation_and_type_effects (&vr,
> -                                            &op_res,
> +         ipa_vr_operation_and_type_effects (vr,
> +                                            op_res,
>                                              NOP_EXPR, param_type,
>                                              operand_type);
>         }
> @@ -2786,14 +2795,14 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>         {
>           if (jfunc->m_vr)
>             {
> -             value_range jvr;
> -             if (ipa_vr_operation_and_type_effects (&jvr, jfunc->m_vr,
> +             Value_Range jvr (param_type);
> +             if (ipa_vr_operation_and_type_effects (jvr, *jfunc->m_vr,
>                                                      NOP_EXPR,
>                                                      param_type,
>                                                      jfunc->m_vr->type ()))
>                 vr.intersect (jvr);
>             }
> -         return dest_lat->meet_with (&vr);
> +         return dest_lat->meet_with (vr);
>         }
>      }
>    else if (jfunc->type == IPA_JF_CONST)
> @@ -2805,20 +2814,19 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>           if (TREE_OVERFLOW_P (val))
>             val = drop_tree_overflow (val);
>
> -         value_range tmpvr (TREE_TYPE (val),
> -                            wi::to_wide (val), wi::to_wide (val));
> -         return dest_lat->meet_with (&tmpvr);
> +         Value_Range tmpvr (val, val);
> +         return dest_lat->meet_with (tmpvr);
>         }
>      }
>
>    value_range vr;
>    if (jfunc->m_vr
> -      && ipa_vr_operation_and_type_effects (&vr, jfunc->m_vr, NOP_EXPR,
> +      && ipa_vr_operation_and_type_effects (vr, *jfunc->m_vr, NOP_EXPR,
>                                             param_type,
>                                             jfunc->m_vr->type ()))
> -    return dest_lat->meet_with (&vr);
> +    return dest_lat->meet_with (vr);
>    else
> -    return dest_lat->set_to_bottom ();
> +    return dest_lat->set_to_bottom (param_type);
>  }
>
>  /* If DEST_PLATS already has aggregate items, check that aggs_by_ref matches
> @@ -3209,7 +3217,8 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>      {
>        for (i = 0; i < parms_count; i++)
>         ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
> -                                                                i));
> +                                                                i),
> +                                         ipa_get_type (callee_info, i));
>        return ret;
>      }
>    args_count = ipa_get_cs_argument_count (args);
> @@ -3220,7 +3229,8 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>    if (call_passes_through_thunk (cs))
>      {
>        ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
> -                                                              0));
> +                                                              0),
> +                                       ipa_get_type (callee_info, 0));
>        i = 1;
>      }
>    else
> @@ -3234,7 +3244,7 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>
>        dest_plats = ipa_get_parm_lattices (callee_info, i);
>        if (availability == AVAIL_INTERPOSABLE)
> -       ret |= set_all_contains_variable (dest_plats);
> +       ret |= set_all_contains_variable (dest_plats, param_type);
>        else
>         {
>           ret |= propagate_scalar_across_jump_function (cs, jump_func,
> @@ -3251,11 +3261,12 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>             ret |= propagate_vr_across_jump_function (cs, jump_func,
>                                                       dest_plats, param_type);
>           else
> -           ret |= dest_plats->m_value_range.set_to_bottom ();
> +           ret |= dest_plats->m_value_range.set_to_bottom (param_type);
>         }
>      }
>    for (; i < parms_count; i++)
> -    ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, i));
> +    ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, i),
> +                                     ipa_get_type (callee_info, i));
>
>    return ret;
>  }
> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> index b328bb8ce14..0474af8991e 100644
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> @@ -475,7 +475,7 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
>           && !c->agg_contents
>           && (!val || TREE_CODE (val) != INTEGER_CST))
>         {
> -         value_range vr = avals->m_known_value_ranges[c->operand_num];
> +         Value_Range vr (avals->m_known_value_ranges[c->operand_num]);
>           if (!vr.undefined_p ()
>               && !vr.varying_p ()
>               && (TYPE_SIZE (c->type) == TYPE_SIZE (vr.type ())))
> @@ -630,8 +630,8 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>                 || ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>               {
>                 /* Determine if we know constant value of the parameter.  */
> -               tree cst = ipa_value_from_jfunc (caller_parms_info, jf,
> -                                                ipa_get_type (callee_pi, i));
> +               tree type = ipa_get_type (callee_pi, i);
> +               tree cst = ipa_value_from_jfunc (caller_parms_info, jf, type);
>
>                 if (!cst && e->call_stmt
>                     && i < (int)gimple_call_num_args (e->call_stmt))
> @@ -659,10 +659,10 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>                     && vrp_will_run_p (caller)
>                     && ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>                   {
> -                   value_range vr
> -                      = ipa_value_range_from_jfunc (caller_parms_info, e, jf,
> -                                                    ipa_get_type (callee_pi,
> -                                                                  i));
> +                   Value_Range vr (type);
> +
> +                   ipa_value_range_from_jfunc (vr, caller_parms_info, e, jf,
> +                                               ipa_get_type (callee_pi, i));
>                     if (!vr.undefined_p () && !vr.varying_p ())
>                       {
>                         if (!avals->m_known_value_ranges.length ())
> @@ -670,7 +670,7 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>                             avals->m_known_value_ranges.safe_grow (count, true);
>                             for (int i = 0; i < count; ++i)
>                               new (&avals->m_known_value_ranges[i])
> -                               value_range ();
> +                               Value_Range ();
>                           }
>                         avals->m_known_value_ranges[i] = vr;
>                       }
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index 4ace410de49..1a71d7969ea 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -5939,7 +5939,7 @@ ipcp_update_vr (struct cgraph_node *node)
>        if (vr[i].known_p ())
>         {
>           tree type = TREE_TYPE (ddef);
> -         value_range tmp;
> +         Value_Range tmp (type);
>           vr[i].get_vrange (tmp, type);
>
>           if (!tmp.undefined_p () && !tmp.varying_p ())
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index 3b580ebb903..3921e33940d 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -530,7 +530,7 @@ public:
>    auto_vec<ipa_argagg_value, 32> m_known_aggs;
>
>    /* Vector describing known value ranges of arguments.  */
> -  auto_vec<value_range, 32> m_known_value_ranges;
> +  auto_vec<Value_Range, 32> m_known_value_ranges;
>  };
>
>  inline
> @@ -582,7 +582,7 @@ public:
>    vec<ipa_argagg_value> m_known_aggs = vNULL;
>
>    /* Vector describing known value ranges of arguments.  */
> -  vec<value_range> m_known_value_ranges = vNULL;
> +  vec<Value_Range> m_known_value_ranges = vNULL;
>  };
>
>  inline
> @@ -1194,8 +1194,8 @@ ipa_polymorphic_call_context ipa_context_from_jfunc (ipa_node_params *,
>                                                      cgraph_edge *,
>                                                      int,
>                                                      ipa_jump_func *);
> -value_range ipa_value_range_from_jfunc (ipa_node_params *, cgraph_edge *,
> -                                       ipa_jump_func *, tree);
> +void ipa_value_range_from_jfunc (vrange &, ipa_node_params *, cgraph_edge *,
> +                                ipa_jump_func *, tree);
>  void ipa_push_agg_values_from_jfunc (ipa_node_params *info, cgraph_node *node,
>                                      ipa_agg_jump_function *agg_jfunc,
>                                      unsigned dst_index,
> @@ -1218,17 +1218,12 @@ void ipa_cp_cc_finalize (void);
>     non-zero.  */
>
>  inline void
> -ipa_range_set_and_normalize (irange &r, tree val)
> +ipa_range_set_and_normalize (vrange &r, tree val)
>  {
> -  if (TREE_CODE (val) == INTEGER_CST)
> -    {
> -      wide_int w = wi::to_wide (val);
> -      r.set (TREE_TYPE (val), w, w);
> -    }
> -  else if (TREE_CODE (val) == ADDR_EXPR)
> +  if (TREE_CODE (val) == ADDR_EXPR)
>      r.set_nonzero (TREE_TYPE (val));
>    else
> -    r.set_varying (TREE_TYPE (val));
> +    r.set (val, val);
>  }
>
>  #endif /* IPA_PROP_H */
> --
> 2.40.0
>

Hello,

On Mon, May 22 2023, Aldy Hernandez wrote:
> I've adjusted the patch with some minor cleanups that came up when I
> implemented the rest of the IPA revamp.
>
> Rested.  OK?
>
> On Wed, May 17, 2023 at 4:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>> This converts the lattice to store ranges in Value_Range instead of
>> value_range (*) to make it type agnostic, and adjust all users
>> accordingly.
>>
>> I think it is a good example on converting from static ranges to more
>> general, type agnostic ones.
>>
>> I've been careful to make sure Value_Range never ends up on GC, since
>> it contains an int_range_max and can expand on-demand onto the heap.
>> Longer term storage for ranges should be done with vrange_storage, as
>> per the previous patch ("Provide an API for ipa_vr").
>>
>> (*) I do know the Value_Range naming versus value_range is quite
>> annoying, but it was a judgement call last release for the eventual
>> migration to having "value_range" be a type agnostic range object.  We
>> will ultimately rename Value_Range to value_range.

It is quite confusing for an unsuspecting reader indeed.

>>
>> OK for trunk?

I guess I need to rely on that you know what you are doing :-)
I have seen in other messages that you measure the compile time
effects of your patches, do you look at memory use as well?

I am happy with the overall approach, I just have the following
comments, questions and a few concerns:


>>
>> gcc/ChangeLog:
>>
>>         * ipa-cp.cc (ipcp_vr_lattice::init): Take type argument.
>>         (ipcp_vr_lattice::print): Call dump method.
>>         (ipcp_vr_lattice::meet_with): Adjust for m_vr being a
>>         Value_Range.
>>         (ipcp_vr_lattice::meet_with_1): Make argument a reference.
>>         (ipcp_vr_lattice::set_to_bottom): Add type argument.
>>         (set_all_contains_variable): Same.
>>         (initialize_node_lattices): Pass type when appropriate.
>>         (ipa_vr_operation_and_type_effects): Make type agnostic.
>>         (ipa_value_range_from_jfunc): Same.
>>         (propagate_vr_across_jump_function): Same.
>>         (propagate_constants_across_call): Same.
>>         * ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same.
>>         (evaluate_properties_for_edge): Same.
>>         * ipa-prop.cc (ipcp_update_vr): Same.
>>         * ipa-prop.h (ipa_value_range_from_jfunc): Same.
>>         (ipa_range_set_and_normalize): Same.
>> ---
>>  gcc/ipa-cp.cc        | 159 +++++++++++++++++++++++--------------------
>>  gcc/ipa-fnsummary.cc |  16 ++---
>>  gcc/ipa-prop.cc      |   2 +-
>>  gcc/ipa-prop.h       |  19 ++----
>>  4 files changed, 101 insertions(+), 95 deletions(-)
>>
>> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
>> index d4b9d4ac27e..bd5b1da17b2 100644
>> --- a/gcc/ipa-cp.cc
>> +++ b/gcc/ipa-cp.cc
>> @@ -343,20 +343,29 @@ private:
>>  class ipcp_vr_lattice
>>  {
>>  public:
>> -  value_range m_vr;
>> +  Value_Range m_vr;
>>
>>    inline bool bottom_p () const;
>>    inline bool top_p () const;
>> -  inline bool set_to_bottom ();
>> -  bool meet_with (const value_range *p_vr);
>> +  inline bool set_to_bottom (tree type);

Requiring a type when setting a lattice to bottom makes for a weird
interface, can't we set the underlying Value_Range to whatever...

>> +  bool meet_with (const vrange &p_vr);
>>    bool meet_with (const ipcp_vr_lattice &other);
>> -  void init () { gcc_assert (m_vr.undefined_p ()); }
>> +  void init (tree type);
>>    void print (FILE * f);
>>
>>  private:
>> -  bool meet_with_1 (const value_range *other_vr);
>> +  bool meet_with_1 (const vrange &other_vr);
>>  };
>>
>> +inline void
>> +ipcp_vr_lattice::init (tree type)
>> +{
>> +  if (type)
>> +    m_vr.set_type (type);
>> +
>> +  // Otherwise m_vr will default to unsupported_range.

...this does?

All users of the lattice check it for not being bottom first, so it
should be safe.

If it is not possible for some reason, then I guess we should add a bool
flag to ipcp_vr_lattice instead, rather than looking up types of
unusable lattices.  ipcp_vr_lattices don't live for long.

>> +}
>> +
>>  /* Structure containing lattices for a parameter itself and for pieces of
>>     aggregates that are passed in the parameter or by a reference in a parameter
>>     plus some other useful flags.  */
>> @@ -585,7 +594,7 @@ ipcp_bits_lattice::print (FILE *f)
>>  void
>>  ipcp_vr_lattice::print (FILE * f)
>>  {
>> -  dump_value_range (f, &m_vr);
>> +  m_vr.dump (f);
>>  }
>>
>>  /* Print all ipcp_lattices of all functions to F.  */
>> @@ -1016,14 +1025,14 @@ set_agg_lats_contain_variable (class ipcp_param_lattices *plats)
>>  bool
>>  ipcp_vr_lattice::meet_with (const ipcp_vr_lattice &other)
>>  {
>> -  return meet_with_1 (&other.m_vr);
>> +  return meet_with_1 (other.m_vr);
>>  }
>>
>>  /* Meet the current value of the lattice with value range described by VR
>>     lattice.  */
>>
>>  bool
>> -ipcp_vr_lattice::meet_with (const value_range *p_vr)
>> +ipcp_vr_lattice::meet_with (const vrange &p_vr)
>>  {
>>    return meet_with_1 (p_vr);
>>  }
>> @@ -1032,23 +1041,23 @@ ipcp_vr_lattice::meet_with (const value_range *p_vr)
>>     OTHER_VR lattice.  Return TRUE if anything changed.  */
>>
>>  bool
>> -ipcp_vr_lattice::meet_with_1 (const value_range *other_vr)
>> +ipcp_vr_lattice::meet_with_1 (const vrange &other_vr)
>>  {
>>    if (bottom_p ())
>>      return false;
>>
>> -  if (other_vr->varying_p ())
>> -    return set_to_bottom ();
>> +  if (other_vr.varying_p ())
>> +    return set_to_bottom (other_vr.type ());
>>
>>    bool res;
>>    if (flag_checking)
>>      {
>> -      value_range save (m_vr);
>> -      res = m_vr.union_ (*other_vr);
>> +      Value_Range save (m_vr);
>> +      res = m_vr.union_ (other_vr);
>>        gcc_assert (res == (m_vr != save));
>>      }
>>    else
>> -    res = m_vr.union_ (*other_vr);
>> +    res = m_vr.union_ (other_vr);
>>    return res;
>>  }
>>
>> @@ -1073,16 +1082,11 @@ ipcp_vr_lattice::bottom_p () const
>>     previously was in a different state.  */
>>
>>  bool
>> -ipcp_vr_lattice::set_to_bottom ()
>> +ipcp_vr_lattice::set_to_bottom (tree type)
>>  {
>>    if (m_vr.varying_p ())
>>      return false;
>> -  /* ?? We create all sorts of VARYING ranges for floats, structures,
>> -     and other types which we cannot handle as ranges.  We should
>> -     probably avoid handling them throughout the pass, but it's easier
>> -     to create a sensible VARYING here and let the lattice
>> -     propagate.  */
>> -  m_vr.set_varying (integer_type_node);
>> +  m_vr.set_varying (type);
>>    return true;
>>  }
>>
>> @@ -1518,14 +1522,14 @@ intersect_argaggs_with (vec<ipa_argagg_value> &elts,
>>     return true is any of them has not been marked as such so far.  */
>>
>>  static inline bool
>> -set_all_contains_variable (class ipcp_param_lattices *plats)
>> +set_all_contains_variable (class ipcp_param_lattices *plats, tree type)
>>  {

...then functions like these would not need the extra parameter either.

>>    bool ret;
>>    ret = plats->itself.set_contains_variable ();
>>    ret |= plats->ctxlat.set_contains_variable ();
>>    ret |= set_agg_lats_contain_variable (plats);
>>    ret |= plats->bits_lattice.set_to_bottom ();
>> -  ret |= plats->m_value_range.set_to_bottom ();
>> +  ret |= plats->m_value_range.set_to_bottom (type);
>>    return ret;
>>  }
>>
>> @@ -1653,6 +1657,7 @@ initialize_node_lattices (struct cgraph_node *node)
>>    for (i = 0; i < ipa_get_param_count (info); i++)
>>      {
>>        ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
>> +      tree type = ipa_get_type (info, i);
>>        if (disable
>>           || !ipa_get_type (info, i)
>>           || (pre_modified && (surviving_params.length () <= (unsigned) i
>> @@ -1662,14 +1667,14 @@ initialize_node_lattices (struct cgraph_node *node)
>>           plats->ctxlat.set_to_bottom ();
>>           set_agg_lats_to_bottom (plats);
>>           plats->bits_lattice.set_to_bottom ();
>> -         plats->m_value_range.m_vr = value_range ();
>> -         plats->m_value_range.set_to_bottom ();
>> +         plats->m_value_range.init (type);
>> +         plats->m_value_range.set_to_bottom (type);
>>         }
>>        else
>>         {
>> -         plats->m_value_range.init ();
>> +         plats->m_value_range.init (type);
>>           if (variable)
>> -           set_all_contains_variable (plats);
>> +           set_all_contains_variable (plats, type);
>>         }
>>      }
>>
>> @@ -1903,8 +1908,8 @@ ipa_context_from_jfunc (ipa_node_params *info, cgraph_edge *cs, int csidx,
>>     the result is a range or an anti-range.  */
>>
>>  static bool
>> -ipa_vr_operation_and_type_effects (value_range *dst_vr,
>> -                                  value_range *src_vr,
>> +ipa_vr_operation_and_type_effects (vrange &dst_vr,
>> +                                  const vrange &src_vr,
>>                                    enum tree_code operation,
>>                                    tree dst_type, tree src_type)
>>  {
>> @@ -1912,29 +1917,33 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr,
>>      return false;
>>
>>    range_op_handler handler (operation, dst_type);
>> -  return (handler
>> -         && handler.fold_range (*dst_vr, dst_type,
>> -                                *src_vr, value_range (dst_type))
>> -         && !dst_vr->varying_p ()
>> -         && !dst_vr->undefined_p ());
>> +  if (!handler)
>> +    return false;
>> +
>> +  Value_Range varying (dst_type);
>> +  varying.set_varying (dst_type);
>> +
>> +  return (handler.fold_range (dst_vr, dst_type, src_vr, varying)
>> +         && !dst_vr.varying_p ()
>> +         && !dst_vr.undefined_p ());
>>  }
>>
>>  /* Determine value_range of JFUNC given that INFO describes the caller node or
>>     the one it is inlined to, CS is the call graph edge corresponding to JFUNC
>>     and PARM_TYPE of the parameter.  */
>>
>> -value_range
>> -ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>> +void
>> +ipa_value_range_from_jfunc (vrange &vr,
>> +                           ipa_node_params *info, cgraph_edge *cs,
>>                             ipa_jump_func *jfunc, tree parm_type)

I assume that you decided to return the value in a parameter passed by
reference instead of in return value for a good reason but then can we
at least...


>>  {
>> -  value_range vr;
>>    if (jfunc->m_vr)
>> -    ipa_vr_operation_and_type_effects (&vr,
>> -                                      jfunc->m_vr,
>> +    ipa_vr_operation_and_type_effects (vr,
>> +                                      *jfunc->m_vr,
>>                                        NOP_EXPR, parm_type,
>>                                        jfunc->m_vr->type ());
>>    if (vr.singleton_p ())
>> -    return vr;
>> +    return;

...make sure that whenever the function intends to return a varying VR
it actually does so instead of not touching it at all?

>>    if (jfunc->type == IPA_JF_PASS_THROUGH)
>>      {
>>        int idx;
>> @@ -1943,33 +1952,34 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>                                            ? cs->caller->inlined_to
>>                                            : cs->caller);
>>        if (!sum || !sum->m_vr)
>> -       return vr;
>> +       return;

Likewise.

>>
>>        idx = ipa_get_jf_pass_through_formal_id (jfunc);
>>
>>        if (!(*sum->m_vr)[idx].known_p ())
>> -       return vr;
>> +       return;

Likewise.

>>        tree vr_type = ipa_get_type (info, idx);
>> -      value_range srcvr;
>> +      Value_Range srcvr (vr_type);
>>        (*sum->m_vr)[idx].get_vrange (srcvr, vr_type);
>>
>>        enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>>
>>        if (TREE_CODE_CLASS (operation) == tcc_unary)
>>         {
>> -         value_range res;
>> +         Value_Range res (vr_type);
>>
>> -         if (ipa_vr_operation_and_type_effects (&res,
>> -                                                &srcvr,
>> +         if (ipa_vr_operation_and_type_effects (res,
>> +                                                srcvr,
>>                                                  operation, parm_type,
>>                                                  vr_type))
>>             vr.intersect (res);

Here we also now make assumptions about the state of vr which we did not
before, should we perhaps assign res into vr instead?

>>         }
>>        else
>>         {
>> -         value_range op_res, res;
>> +         Value_Range op_res (vr_type);
>> +         Value_Range res (vr_type);
>>           tree op = ipa_get_jf_pass_through_operand (jfunc);
>> -         value_range op_vr;
>> +         Value_Range op_vr (vr_type);
>>           range_op_handler handler (operation, vr_type);
>>
>>           ipa_range_set_and_normalize (op_vr, op);
>> @@ -1979,14 +1989,13 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>               || !handler.fold_range (op_res, vr_type, srcvr, op_vr))
>>             op_res.set_varying (vr_type);
>>
>> -         if (ipa_vr_operation_and_type_effects (&res,
>> -                                                &op_res,
>> +         if (ipa_vr_operation_and_type_effects (res,
>> +                                                op_res,
>>                                                  NOP_EXPR, parm_type,
>>                                                  vr_type))
>>             vr.intersect (res);

Likewise.

>>         }
>>      }
>> -  return vr;
>>  }
>>
>>  /* Determine whether ITEM, jump function for an aggregate part, evaluates to a
>> @@ -2739,7 +2748,7 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>    if (!param_type
>>        || (!INTEGRAL_TYPE_P (param_type)
>>           && !POINTER_TYPE_P (param_type)))
>> -    return dest_lat->set_to_bottom ();
>> +    return dest_lat->set_to_bottom (param_type);
>>
>>    if (jfunc->type == IPA_JF_PASS_THROUGH)
>>      {
>> @@ -2751,12 +2760,12 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>        tree operand_type = ipa_get_type (caller_info, src_idx);
>>
>>        if (src_lats->m_value_range.bottom_p ())
>> -       return dest_lat->set_to_bottom ();
>> +       return dest_lat->set_to_bottom (operand_type);
>>
>> -      value_range vr;
>> +      Value_Range vr (operand_type);
>>        if (TREE_CODE_CLASS (operation) == tcc_unary)
>> -       ipa_vr_operation_and_type_effects (&vr,
>> -                                          &src_lats->m_value_range.m_vr,
>> +       ipa_vr_operation_and_type_effects (vr,
>> +                                          src_lats->m_value_range.m_vr,
>>                                            operation, param_type,
>>                                            operand_type);
>>        /* A crude way to prevent unbounded number of value range updates
>> @@ -2765,8 +2774,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>        else if (!ipa_edge_within_scc (cs))
>>         {
>>           tree op = ipa_get_jf_pass_through_operand (jfunc);
>> -         value_range op_vr;
>> -         value_range op_res,res;
>> +         Value_Range op_vr (TREE_TYPE (op));
>> +         Value_Range op_res (operand_type);
>>           range_op_handler handler (operation, operand_type);
>>
>>           ipa_range_set_and_normalize (op_vr, op);
>> @@ -2777,8 +2786,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>                                       src_lats->m_value_range.m_vr, op_vr))
>>             op_res.set_varying (operand_type);
>>
>> -         ipa_vr_operation_and_type_effects (&vr,
>> -                                            &op_res,
>> +         ipa_vr_operation_and_type_effects (vr,
>> +                                            op_res,
>>                                              NOP_EXPR, param_type,
>>                                              operand_type);
>>         }
>> @@ -2786,14 +2795,14 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>         {
>>           if (jfunc->m_vr)
>>             {
>> -             value_range jvr;
>> -             if (ipa_vr_operation_and_type_effects (&jvr, jfunc->m_vr,
>> +             Value_Range jvr (param_type);
>> +             if (ipa_vr_operation_and_type_effects (jvr, *jfunc->m_vr,
>>                                                      NOP_EXPR,
>>                                                      param_type,
>>                                                      jfunc->m_vr->type ()))
>>                 vr.intersect (jvr);
>>             }
>> -         return dest_lat->meet_with (&vr);
>> +         return dest_lat->meet_with (vr);
>>         }
>>      }
>>    else if (jfunc->type == IPA_JF_CONST)
>> @@ -2805,20 +2814,19 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>           if (TREE_OVERFLOW_P (val))
>>             val = drop_tree_overflow (val);
>>
>> -         value_range tmpvr (TREE_TYPE (val),
>> -                            wi::to_wide (val), wi::to_wide (val));
>> -         return dest_lat->meet_with (&tmpvr);
>> +         Value_Range tmpvr (val, val);
>> +         return dest_lat->meet_with (tmpvr);
>>         }
>>      }
>>
>>    value_range vr;
>>    if (jfunc->m_vr
>> -      && ipa_vr_operation_and_type_effects (&vr, jfunc->m_vr, NOP_EXPR,
>> +      && ipa_vr_operation_and_type_effects (vr, *jfunc->m_vr, NOP_EXPR,
>>                                             param_type,
>>                                             jfunc->m_vr->type ()))
>> -    return dest_lat->meet_with (&vr);
>> +    return dest_lat->meet_with (vr);
>>    else
>> -    return dest_lat->set_to_bottom ();
>> +    return dest_lat->set_to_bottom (param_type);
>>  }
>>
>>  /* If DEST_PLATS already has aggregate items, check that aggs_by_ref matches
>> @@ -3209,7 +3217,8 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>>      {
>>        for (i = 0; i < parms_count; i++)
>>         ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
>> -                                                                i));
>> +                                                                i),
>> +                                         ipa_get_type (callee_info, i));

I have complained about it above but this another example where making
ipcp_vr_lattice::set_to_bottom not require a type which is not really
needed could even save a tiny bit of compile time.

>>        return ret;
>>      }
>>    args_count = ipa_get_cs_argument_count (args);
>> @@ -3220,7 +3229,8 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>>    if (call_passes_through_thunk (cs))
>>      {
>>        ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
>> -                                                              0));
>> +                                                              0),
>> +                                       ipa_get_type (callee_info, 0));
>>        i = 1;
>>      }
>>    else
>> @@ -3234,7 +3244,7 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>>
>>        dest_plats = ipa_get_parm_lattices (callee_info, i);
>>        if (availability == AVAIL_INTERPOSABLE)
>> -       ret |= set_all_contains_variable (dest_plats);
>> +       ret |= set_all_contains_variable (dest_plats, param_type);
>>        else
>>         {
>>           ret |= propagate_scalar_across_jump_function (cs, jump_func,
>> @@ -3251,11 +3261,12 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>>             ret |= propagate_vr_across_jump_function (cs, jump_func,
>>                                                       dest_plats, param_type);
>>           else
>> -           ret |= dest_plats->m_value_range.set_to_bottom ();
>> +           ret |= dest_plats->m_value_range.set_to_bottom (param_type);
>>         }
>>      }
>>    for (; i < parms_count; i++)
>> -    ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, i));
>> +    ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, i),
>> +                                     ipa_get_type (callee_info, i));
>>
>>    return ret;
>>  }
>> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
>> index b328bb8ce14..0474af8991e 100644
>> --- a/gcc/ipa-fnsummary.cc
>> +++ b/gcc/ipa-fnsummary.cc
>> @@ -475,7 +475,7 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
>>           && !c->agg_contents
>>           && (!val || TREE_CODE (val) != INTEGER_CST))
>>         {
>> -         value_range vr = avals->m_known_value_ranges[c->operand_num];
>> +         Value_Range vr (avals->m_known_value_ranges[c->operand_num]);
>>           if (!vr.undefined_p ()
>>               && !vr.varying_p ()
>>               && (TYPE_SIZE (c->type) == TYPE_SIZE (vr.type ())))
>> @@ -630,8 +630,8 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>                 || ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>>               {
>>                 /* Determine if we know constant value of the parameter.  */
>> -               tree cst = ipa_value_from_jfunc (caller_parms_info, jf,
>> -                                                ipa_get_type (callee_pi, i));
>> +               tree type = ipa_get_type (callee_pi, i);
>> +               tree cst = ipa_value_from_jfunc (caller_parms_info, jf, type);
>>
>>                 if (!cst && e->call_stmt
>>                     && i < (int)gimple_call_num_args (e->call_stmt))
>> @@ -659,10 +659,10 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>                     && vrp_will_run_p (caller)
>>                     && ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>>                   {
>> -                   value_range vr
>> -                      = ipa_value_range_from_jfunc (caller_parms_info, e, jf,
>> -                                                    ipa_get_type (callee_pi,
>> -                                                                  i));
>> +                   Value_Range vr (type);
>> +
>> +                   ipa_value_range_from_jfunc (vr, caller_parms_info, e, jf,
>> +                                               ipa_get_type (callee_pi, i));

I guess that the ipa_get_type call can also be replaced with "type" now.

>>                     if (!vr.undefined_p () && !vr.varying_p ())
>>                       {
>>                         if (!avals->m_known_value_ranges.length ())
>> @@ -670,7 +670,7 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>                             avals->m_known_value_ranges.safe_grow (count, true);
>>                             for (int i = 0; i < count; ++i)
>>                               new (&avals->m_known_value_ranges[i])
>> -                               value_range ();
>> +                               Value_Range ();
>>                           }
>>                         avals->m_known_value_ranges[i] = vr;
>>                       }

Thanks for working on this and sorry that it takes me so long to review.

Martin

My apologies for the delay.  I was on vacation.

On 5/26/23 18:17, Martin Jambor wrote:
> Hello,
> 
> On Mon, May 22 2023, Aldy Hernandez wrote:
>> I've adjusted the patch with some minor cleanups that came up when I
>> implemented the rest of the IPA revamp.
>>
>> Rested.  OK?
>>
>> On Wed, May 17, 2023 at 4:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>
>>> This converts the lattice to store ranges in Value_Range instead of
>>> value_range (*) to make it type agnostic, and adjust all users
>>> accordingly.
>>>
>>> I think it is a good example on converting from static ranges to more
>>> general, type agnostic ones.
>>>
>>> I've been careful to make sure Value_Range never ends up on GC, since
>>> it contains an int_range_max and can expand on-demand onto the heap.
>>> Longer term storage for ranges should be done with vrange_storage, as
>>> per the previous patch ("Provide an API for ipa_vr").
>>>
>>> (*) I do know the Value_Range naming versus value_range is quite
>>> annoying, but it was a judgement call last release for the eventual
>>> migration to having "value_range" be a type agnostic range object.  We
>>> will ultimately rename Value_Range to value_range.
> 
> It is quite confusing for an unsuspecting reader indeed.
> 
>>>
>>> OK for trunk?
> 
> I guess I need to rely on that you know what you are doing :-)
> I have seen in other messages that you measure the compile time
> effects of your patches, do you look at memory use as well?

Before going in depth into the rest of your review (thanks BTW), let's 
address memory usage.

To be honest, I didn't measure memory, but only because I had a pretty 
good inkling that it wouldn't make a difference, since vrange_storage is 
designed to take less space than value_range which you were using.  But 
you're right, I should've measured it.

First value_range is a derived-POD so it has a vtable pointer in there. 
vrange_storage does not.

Second, vrange_storage only uses the minimum number of bytes to 
represent the integers in the range.  vrange_storage uses a 
trailing_wide_int type mechanism to store the minimum amount of HWIs for 
a range.   See irange_storage::set_irange().

value_range is a typedef for int_range<2>.  Storing this in GC, which 
IPA currently does, takes 432 bytes.  I will be removing the GTY markers 
for irange, to keep anyone from even trying.

On the other hand, storing this same range in GC memory with 
vrange_storage takes 35 bytes for a [5, 10] range:

   unsigned prec = TYPE_PRECISION (integer_type_node);
   wide_int min = wi::shwi (5, prec);
   wide_int max = wi::shwi (10, prec);
   int_range<2> r (integer_type_node, min, max);
   vrange_storage *p = ggc_alloc_vrange_storage (r);

Breaking on irange_storage::alloc() you can see the size of the 
allocated object is 35 bytes.  This is far less than 432 bytes in trunk 
(due to the wide_int penalty in this release), but even so is comparable 
to GCC12 which took 32 bytes.  Note that GCC12 was using trees, so those 
32 bytes were deceptive, since there was another level of indirection 
involved for the actual HWIs.

Anywhoo... I tried comparing GCC 12 to current mainline plus these 
patches, but it was a royal PITA, because so much has changed, not just 
in the ranger world, but in the rest of the compiler.

However, comparing trunk against my patches is a total wash, even 
considering that IPA will now store a gazillion more ranges.

For measuring I built with --enable-gather-detailed-mem-stats and 
compared the "Total Allocated:" lines from -fmem-report for the 
aggregate of all .ii files in a bootstrap:

Before:
Total allocated: 73360474112.0 bytes

After:
Total allocated: 73354182656.0 bytes

So we use 0.00858% less memory.

To be honest, this is an unfair comparison because trunk IPA is 
streaming out the full value_range (wide_int's and all which changed in 
this release), but with these patches:

a) We don't stream out vtable pointer.
b) Even if the number of sub-ranges can be larger, we only store the 
bare minimum, and we're capped at 255 sub-ranges (which I've never seen 
in the wild).

I think we're good, but if this ever becomes a problem, the constructor 
for ipa_vr could just be tweaked to squish things down before allocating:

ipa_vr::ipa_vr (const vrange &v)
   : m_type (v.type ())
{
   if (is_a <irange> (v))
     {
       int_range<10> squish (as_a <irange> (v));
       m_storage = ggc_alloc_vrange_storage (squish);
     }
   else
     m_storage = ggc_alloc_vrange_storage (v);
}

I'll address the rest of your comments in follow-up mails.
Aldy

On 5/26/23 18:17, Martin Jambor wrote:
> Hello,
> 
> On Mon, May 22 2023, Aldy Hernandez wrote:
>> I've adjusted the patch with some minor cleanups that came up when I
>> implemented the rest of the IPA revamp.
>>
>> Rested.  OK?
>>
>> On Wed, May 17, 2023 at 4:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>
>>> This converts the lattice to store ranges in Value_Range instead of
>>> value_range (*) to make it type agnostic, and adjust all users
>>> accordingly.
>>>
>>> I think it is a good example on converting from static ranges to more
>>> general, type agnostic ones.
>>>
>>> I've been careful to make sure Value_Range never ends up on GC, since
>>> it contains an int_range_max and can expand on-demand onto the heap.
>>> Longer term storage for ranges should be done with vrange_storage, as
>>> per the previous patch ("Provide an API for ipa_vr").
>>>
>>> (*) I do know the Value_Range naming versus value_range is quite
>>> annoying, but it was a judgement call last release for the eventual
>>> migration to having "value_range" be a type agnostic range object.  We
>>> will ultimately rename Value_Range to value_range.
> 
> It is quite confusing for an unsuspecting reader indeed.
> 
>>>
>>> OK for trunk?
> 
> I guess I need to rely on that you know what you are doing :-)

I wouldn't go that far ;-).

> I have seen in other messages that you measure the compile time
> effects of your patches, do you look at memory use as well?

As per my message yesterday, the memory usage seems reasonable.

> 
> I am happy with the overall approach, I just have the following
> comments, questions and a few concerns:
> 
> 
>>>
>>> gcc/ChangeLog:
>>>
>>>          * ipa-cp.cc (ipcp_vr_lattice::init): Take type argument.
>>>          (ipcp_vr_lattice::print): Call dump method.
>>>          (ipcp_vr_lattice::meet_with): Adjust for m_vr being a
>>>          Value_Range.
>>>          (ipcp_vr_lattice::meet_with_1): Make argument a reference.
>>>          (ipcp_vr_lattice::set_to_bottom): Add type argument.
>>>          (set_all_contains_variable): Same.
>>>          (initialize_node_lattices): Pass type when appropriate.
>>>          (ipa_vr_operation_and_type_effects): Make type agnostic.
>>>          (ipa_value_range_from_jfunc): Same.
>>>          (propagate_vr_across_jump_function): Same.
>>>          (propagate_constants_across_call): Same.
>>>          * ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same.
>>>          (evaluate_properties_for_edge): Same.
>>>          * ipa-prop.cc (ipcp_update_vr): Same.
>>>          * ipa-prop.h (ipa_value_range_from_jfunc): Same.
>>>          (ipa_range_set_and_normalize): Same.
>>> ---
>>>   gcc/ipa-cp.cc        | 159 +++++++++++++++++++++++--------------------
>>>   gcc/ipa-fnsummary.cc |  16 ++---
>>>   gcc/ipa-prop.cc      |   2 +-
>>>   gcc/ipa-prop.h       |  19 ++----
>>>   4 files changed, 101 insertions(+), 95 deletions(-)
>>>
>>> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
>>> index d4b9d4ac27e..bd5b1da17b2 100644
>>> --- a/gcc/ipa-cp.cc
>>> +++ b/gcc/ipa-cp.cc
>>> @@ -343,20 +343,29 @@ private:
>>>   class ipcp_vr_lattice
>>>   {
>>>   public:
>>> -  value_range m_vr;
>>> +  Value_Range m_vr;
>>>
>>>     inline bool bottom_p () const;
>>>     inline bool top_p () const;
>>> -  inline bool set_to_bottom ();
>>> -  bool meet_with (const value_range *p_vr);
>>> +  inline bool set_to_bottom (tree type);
> 
> Requiring a type when setting a lattice to bottom makes for a weird
> interface, can't we set the underlying Value_Range to whatever... >
>>> +  bool meet_with (const vrange &p_vr);
>>>     bool meet_with (const ipcp_vr_lattice &other);
>>> -  void init () { gcc_assert (m_vr.undefined_p ()); }
>>> +  void init (tree type);
>>>     void print (FILE * f);
>>>
>>>   private:
>>> -  bool meet_with_1 (const value_range *other_vr);
>>> +  bool meet_with_1 (const vrange &other_vr);
>>>   };
>>>
>>> +inline void
>>> +ipcp_vr_lattice::init (tree type)
>>> +{
>>> +  if (type)
>>> +    m_vr.set_type (type);
>>> +
>>> +  // Otherwise m_vr will default to unsupported_range.
> 
> ...this does?
> 
> All users of the lattice check it for not being bottom first, so it
> should be safe.
> 
> If it is not possible for some reason, then I guess we should add a bool
> flag to ipcp_vr_lattice instead, rather than looking up types of
> unusable lattices.  ipcp_vr_lattices don't live for long.

The type was my least favorite part of this work.  And yes, your 
suggestion would work.  I have tweaked the patch to force a VARYING for 
an unsupported range which seems to do the trick.  It looks much 
cleaner.  Thanks.

> 
>>> +}
>>> +
>>>   /* Structure containing lattices for a parameter itself and for pieces of
>>>      aggregates that are passed in the parameter or by a reference in a parameter
>>>      plus some other useful flags.  */
>>> @@ -585,7 +594,7 @@ ipcp_bits_lattice::print (FILE *f)
>>>   void
>>>   ipcp_vr_lattice::print (FILE * f)
>>>   {
>>> -  dump_value_range (f, &m_vr);
>>> +  m_vr.dump (f);
>>>   }
>>>
>>>   /* Print all ipcp_lattices of all functions to F.  */
>>> @@ -1016,14 +1025,14 @@ set_agg_lats_contain_variable (class ipcp_param_lattices *plats)
>>>   bool
>>>   ipcp_vr_lattice::meet_with (const ipcp_vr_lattice &other)
>>>   {
>>> -  return meet_with_1 (&other.m_vr);
>>> +  return meet_with_1 (other.m_vr);
>>>   }
>>>
>>>   /* Meet the current value of the lattice with value range described by VR
>>>      lattice.  */
>>>
>>>   bool
>>> -ipcp_vr_lattice::meet_with (const value_range *p_vr)
>>> +ipcp_vr_lattice::meet_with (const vrange &p_vr)
>>>   {
>>>     return meet_with_1 (p_vr);
>>>   }
>>> @@ -1032,23 +1041,23 @@ ipcp_vr_lattice::meet_with (const value_range *p_vr)
>>>      OTHER_VR lattice.  Return TRUE if anything changed.  */
>>>
>>>   bool
>>> -ipcp_vr_lattice::meet_with_1 (const value_range *other_vr)
>>> +ipcp_vr_lattice::meet_with_1 (const vrange &other_vr)
>>>   {
>>>     if (bottom_p ())
>>>       return false;
>>>
>>> -  if (other_vr->varying_p ())
>>> -    return set_to_bottom ();
>>> +  if (other_vr.varying_p ())
>>> +    return set_to_bottom (other_vr.type ());
>>>
>>>     bool res;
>>>     if (flag_checking)
>>>       {
>>> -      value_range save (m_vr);
>>> -      res = m_vr.union_ (*other_vr);
>>> +      Value_Range save (m_vr);
>>> +      res = m_vr.union_ (other_vr);
>>>         gcc_assert (res == (m_vr != save));
>>>       }
>>>     else
>>> -    res = m_vr.union_ (*other_vr);
>>> +    res = m_vr.union_ (other_vr);
>>>     return res;
>>>   }
>>>
>>> @@ -1073,16 +1082,11 @@ ipcp_vr_lattice::bottom_p () const
>>>      previously was in a different state.  */
>>>
>>>   bool
>>> -ipcp_vr_lattice::set_to_bottom ()
>>> +ipcp_vr_lattice::set_to_bottom (tree type)
>>>   {
>>>     if (m_vr.varying_p ())
>>>       return false;
>>> -  /* ?? We create all sorts of VARYING ranges for floats, structures,
>>> -     and other types which we cannot handle as ranges.  We should
>>> -     probably avoid handling them throughout the pass, but it's easier
>>> -     to create a sensible VARYING here and let the lattice
>>> -     propagate.  */
>>> -  m_vr.set_varying (integer_type_node);
>>> +  m_vr.set_varying (type);
>>>     return true;
>>>   }
>>>
>>> @@ -1518,14 +1522,14 @@ intersect_argaggs_with (vec<ipa_argagg_value> &elts,
>>>      return true is any of them has not been marked as such so far.  */
>>>
>>>   static inline bool
>>> -set_all_contains_variable (class ipcp_param_lattices *plats)
>>> +set_all_contains_variable (class ipcp_param_lattices *plats, tree type)
>>>   {
> 
> ...then functions like these would not need the extra parameter either.
> 
>>>     bool ret;
>>>     ret = plats->itself.set_contains_variable ();
>>>     ret |= plats->ctxlat.set_contains_variable ();
>>>     ret |= set_agg_lats_contain_variable (plats);
>>>     ret |= plats->bits_lattice.set_to_bottom ();
>>> -  ret |= plats->m_value_range.set_to_bottom ();
>>> +  ret |= plats->m_value_range.set_to_bottom (type);
>>>     return ret;
>>>   }
>>>
>>> @@ -1653,6 +1657,7 @@ initialize_node_lattices (struct cgraph_node *node)
>>>     for (i = 0; i < ipa_get_param_count (info); i++)
>>>       {
>>>         ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
>>> +      tree type = ipa_get_type (info, i);
>>>         if (disable
>>>            || !ipa_get_type (info, i)
>>>            || (pre_modified && (surviving_params.length () <= (unsigned) i
>>> @@ -1662,14 +1667,14 @@ initialize_node_lattices (struct cgraph_node *node)
>>>            plats->ctxlat.set_to_bottom ();
>>>            set_agg_lats_to_bottom (plats);
>>>            plats->bits_lattice.set_to_bottom ();
>>> -         plats->m_value_range.m_vr = value_range ();
>>> -         plats->m_value_range.set_to_bottom ();
>>> +         plats->m_value_range.init (type);
>>> +         plats->m_value_range.set_to_bottom (type);
>>>          }
>>>         else
>>>          {
>>> -         plats->m_value_range.init ();
>>> +         plats->m_value_range.init (type);
>>>            if (variable)
>>> -           set_all_contains_variable (plats);
>>> +           set_all_contains_variable (plats, type);
>>>          }
>>>       }
>>>
>>> @@ -1903,8 +1908,8 @@ ipa_context_from_jfunc (ipa_node_params *info, cgraph_edge *cs, int csidx,
>>>      the result is a range or an anti-range.  */
>>>
>>>   static bool
>>> -ipa_vr_operation_and_type_effects (value_range *dst_vr,
>>> -                                  value_range *src_vr,
>>> +ipa_vr_operation_and_type_effects (vrange &dst_vr,
>>> +                                  const vrange &src_vr,
>>>                                     enum tree_code operation,
>>>                                     tree dst_type, tree src_type)
>>>   {
>>> @@ -1912,29 +1917,33 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr,
>>>       return false;
>>>
>>>     range_op_handler handler (operation, dst_type);
>>> -  return (handler
>>> -         && handler.fold_range (*dst_vr, dst_type,
>>> -                                *src_vr, value_range (dst_type))
>>> -         && !dst_vr->varying_p ()
>>> -         && !dst_vr->undefined_p ());
>>> +  if (!handler)
>>> +    return false;
>>> +
>>> +  Value_Range varying (dst_type);
>>> +  varying.set_varying (dst_type);
>>> +
>>> +  return (handler.fold_range (dst_vr, dst_type, src_vr, varying)
>>> +         && !dst_vr.varying_p ()
>>> +         && !dst_vr.undefined_p ());
>>>   }
>>>
>>>   /* Determine value_range of JFUNC given that INFO describes the caller node or
>>>      the one it is inlined to, CS is the call graph edge corresponding to JFUNC
>>>      and PARM_TYPE of the parameter.  */
>>>
>>> -value_range
>>> -ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>> +void
>>> +ipa_value_range_from_jfunc (vrange &vr,
>>> +                           ipa_node_params *info, cgraph_edge *cs,
>>>                              ipa_jump_func *jfunc, tree parm_type)
> 
> I assume that you decided to return the value in a parameter passed by
> reference instead of in return value for a good reason but then can we
> at least...

vrange is an abstract type, plus it can be any size (int_range<3> has 3 
sub-ranges, legacy value_range has 2 sub-ranges, frange is a totally 
different object, etc).  Throughout all of ranger, returning a range is 
done by passing by reference.  This has the added benefit that sometimes 
we can set a return range by twiddling a few bits (foo.set_undefined()) 
instead of having to copy a full range back and forth.

> 
> 
>>>   {
>>> -  value_range vr;
>>>     if (jfunc->m_vr)
>>> -    ipa_vr_operation_and_type_effects (&vr,
>>> -                                      jfunc->m_vr,
>>> +    ipa_vr_operation_and_type_effects (vr,
>>> +                                      *jfunc->m_vr,
>>>                                         NOP_EXPR, parm_type,
>>>                                         jfunc->m_vr->type ());
>>>     if (vr.singleton_p ())
>>> -    return vr;
>>> +    return;
> 
> ...make sure that whenever the function intends to return a varying VR
> it actually does so instead of not touching it at all?

Good catch.  The original "value_range vr;" definition would default vr 
to UNDEFINED, which would no longer happen with my patch.  I have added 
a vr.set_undefined() at the original definition site, which would make 
everything work.

> 
>>>     if (jfunc->type == IPA_JF_PASS_THROUGH)
>>>       {
>>>         int idx;
>>> @@ -1943,33 +1952,34 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>>                                             ? cs->caller->inlined_to
>>>                                             : cs->caller);
>>>         if (!sum || !sum->m_vr)
>>> -       return vr;
>>> +       return;
> 
> Likewise.
> 
>>>
>>>         idx = ipa_get_jf_pass_through_formal_id (jfunc);
>>>
>>>         if (!(*sum->m_vr)[idx].known_p ())
>>> -       return vr;
>>> +       return;
> 
> Likewise.
> 
>>>         tree vr_type = ipa_get_type (info, idx);
>>> -      value_range srcvr;
>>> +      Value_Range srcvr (vr_type);
>>>         (*sum->m_vr)[idx].get_vrange (srcvr, vr_type);
>>>
>>>         enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>>>
>>>         if (TREE_CODE_CLASS (operation) == tcc_unary)
>>>          {
>>> -         value_range res;
>>> +         Value_Range res (vr_type);
>>>
>>> -         if (ipa_vr_operation_and_type_effects (&res,
>>> -                                                &srcvr,
>>> +         if (ipa_vr_operation_and_type_effects (res,
>>> +                                                srcvr,
>>>                                                   operation, parm_type,
>>>                                                   vr_type))
>>>              vr.intersect (res);
> 
> Here we also now make assumptions about the state of vr which we did not
> before, should we perhaps assign res into vr instead?

With the initialization of vr to UNDEFINED, this should be OK as both of 
these initialize "res" to UNDEFINED:

 >>> -         value_range res;
 >>> +         Value_Range res (vr_type);

So vr and res both have the same default as before.

> 
>>>          }
>>>         else
>>>          {
>>> -         value_range op_res, res;
>>> +         Value_Range op_res (vr_type);
>>> +         Value_Range res (vr_type);
>>>            tree op = ipa_get_jf_pass_through_operand (jfunc);
>>> -         value_range op_vr;
>>> +         Value_Range op_vr (vr_type);
>>>            range_op_handler handler (operation, vr_type);
>>>
>>>            ipa_range_set_and_normalize (op_vr, op);
>>> @@ -1979,14 +1989,13 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>>                || !handler.fold_range (op_res, vr_type, srcvr, op_vr))
>>>              op_res.set_varying (vr_type);
>>>
>>> -         if (ipa_vr_operation_and_type_effects (&res,
>>> -                                                &op_res,
>>> +         if (ipa_vr_operation_and_type_effects (res,
>>> +                                                op_res,
>>>                                                   NOP_EXPR, parm_type,
>>>                                                   vr_type))
>>>              vr.intersect (res);
> 
> Likewise.
> 
>>>          }
>>>       }
>>> -  return vr;
>>>   }
>>>
>>>   /* Determine whether ITEM, jump function for an aggregate part, evaluates to a
>>> @@ -2739,7 +2748,7 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>>     if (!param_type
>>>         || (!INTEGRAL_TYPE_P (param_type)
>>>            && !POINTER_TYPE_P (param_type)))
>>> -    return dest_lat->set_to_bottom ();
>>> +    return dest_lat->set_to_bottom (param_type);
>>>
>>>     if (jfunc->type == IPA_JF_PASS_THROUGH)
>>>       {
>>> @@ -2751,12 +2760,12 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>>         tree operand_type = ipa_get_type (caller_info, src_idx);
>>>
>>>         if (src_lats->m_value_range.bottom_p ())
>>> -       return dest_lat->set_to_bottom ();
>>> +       return dest_lat->set_to_bottom (operand_type);
>>>
>>> -      value_range vr;
>>> +      Value_Range vr (operand_type);
>>>         if (TREE_CODE_CLASS (operation) == tcc_unary)
>>> -       ipa_vr_operation_and_type_effects (&vr,
>>> -                                          &src_lats->m_value_range.m_vr,
>>> +       ipa_vr_operation_and_type_effects (vr,
>>> +                                          src_lats->m_value_range.m_vr,
>>>                                             operation, param_type,
>>>                                             operand_type);
>>>         /* A crude way to prevent unbounded number of value range updates
>>> @@ -2765,8 +2774,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>>         else if (!ipa_edge_within_scc (cs))
>>>          {
>>>            tree op = ipa_get_jf_pass_through_operand (jfunc);
>>> -         value_range op_vr;
>>> -         value_range op_res,res;
>>> +         Value_Range op_vr (TREE_TYPE (op));
>>> +         Value_Range op_res (operand_type);
>>>            range_op_handler handler (operation, operand_type);
>>>
>>>            ipa_range_set_and_normalize (op_vr, op);
>>> @@ -2777,8 +2786,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>>                                        src_lats->m_value_range.m_vr, op_vr))
>>>              op_res.set_varying (operand_type);
>>>
>>> -         ipa_vr_operation_and_type_effects (&vr,
>>> -                                            &op_res,
>>> +         ipa_vr_operation_and_type_effects (vr,
>>> +                                            op_res,
>>>                                               NOP_EXPR, param_type,
>>>                                               operand_type);
>>>          }
>>> @@ -2786,14 +2795,14 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>>          {
>>>            if (jfunc->m_vr)
>>>              {
>>> -             value_range jvr;
>>> -             if (ipa_vr_operation_and_type_effects (&jvr, jfunc->m_vr,
>>> +             Value_Range jvr (param_type);
>>> +             if (ipa_vr_operation_and_type_effects (jvr, *jfunc->m_vr,
>>>                                                       NOP_EXPR,
>>>                                                       param_type,
>>>                                                       jfunc->m_vr->type ()))
>>>                  vr.intersect (jvr);
>>>              }
>>> -         return dest_lat->meet_with (&vr);
>>> +         return dest_lat->meet_with (vr);
>>>          }
>>>       }
>>>     else if (jfunc->type == IPA_JF_CONST)
>>> @@ -2805,20 +2814,19 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>>            if (TREE_OVERFLOW_P (val))
>>>              val = drop_tree_overflow (val);
>>>
>>> -         value_range tmpvr (TREE_TYPE (val),
>>> -                            wi::to_wide (val), wi::to_wide (val));
>>> -         return dest_lat->meet_with (&tmpvr);
>>> +         Value_Range tmpvr (val, val);
>>> +         return dest_lat->meet_with (tmpvr);
>>>          }
>>>       }
>>>
>>>     value_range vr;
>>>     if (jfunc->m_vr
>>> -      && ipa_vr_operation_and_type_effects (&vr, jfunc->m_vr, NOP_EXPR,
>>> +      && ipa_vr_operation_and_type_effects (vr, *jfunc->m_vr, NOP_EXPR,
>>>                                              param_type,
>>>                                              jfunc->m_vr->type ()))
>>> -    return dest_lat->meet_with (&vr);
>>> +    return dest_lat->meet_with (vr);
>>>     else
>>> -    return dest_lat->set_to_bottom ();
>>> +    return dest_lat->set_to_bottom (param_type);
>>>   }
>>>
>>>   /* If DEST_PLATS already has aggregate items, check that aggs_by_ref matches
>>> @@ -3209,7 +3217,8 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>>>       {
>>>         for (i = 0; i < parms_count; i++)
>>>          ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
>>> -                                                                i));
>>> +                                                                i),
>>> +                                         ipa_get_type (callee_info, i));
> 
> I have complained about it above but this another example where making
> ipcp_vr_lattice::set_to_bottom not require a type which is not really
> needed could even save a tiny bit of compile time.
> 
>>>         return ret;
>>>       }
>>>     args_count = ipa_get_cs_argument_count (args);
>>> @@ -3220,7 +3229,8 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>>>     if (call_passes_through_thunk (cs))
>>>       {
>>>         ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info,
>>> -                                                              0));
>>> +                                                              0),
>>> +                                       ipa_get_type (callee_info, 0));
>>>         i = 1;
>>>       }
>>>     else
>>> @@ -3234,7 +3244,7 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>>>
>>>         dest_plats = ipa_get_parm_lattices (callee_info, i);
>>>         if (availability == AVAIL_INTERPOSABLE)
>>> -       ret |= set_all_contains_variable (dest_plats);
>>> +       ret |= set_all_contains_variable (dest_plats, param_type);
>>>         else
>>>          {
>>>            ret |= propagate_scalar_across_jump_function (cs, jump_func,
>>> @@ -3251,11 +3261,12 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>>>              ret |= propagate_vr_across_jump_function (cs, jump_func,
>>>                                                        dest_plats, param_type);
>>>            else
>>> -           ret |= dest_plats->m_value_range.set_to_bottom ();
>>> +           ret |= dest_plats->m_value_range.set_to_bottom (param_type);
>>>          }
>>>       }
>>>     for (; i < parms_count; i++)
>>> -    ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, i));
>>> +    ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, i),
>>> +                                     ipa_get_type (callee_info, i));
>>>
>>>     return ret;
>>>   }
>>> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
>>> index b328bb8ce14..0474af8991e 100644
>>> --- a/gcc/ipa-fnsummary.cc
>>> +++ b/gcc/ipa-fnsummary.cc
>>> @@ -475,7 +475,7 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
>>>            && !c->agg_contents
>>>            && (!val || TREE_CODE (val) != INTEGER_CST))
>>>          {
>>> -         value_range vr = avals->m_known_value_ranges[c->operand_num];
>>> +         Value_Range vr (avals->m_known_value_ranges[c->operand_num]);
>>>            if (!vr.undefined_p ()
>>>                && !vr.varying_p ()
>>>                && (TYPE_SIZE (c->type) == TYPE_SIZE (vr.type ())))
>>> @@ -630,8 +630,8 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>>                  || ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>>>                {
>>>                  /* Determine if we know constant value of the parameter.  */
>>> -               tree cst = ipa_value_from_jfunc (caller_parms_info, jf,
>>> -                                                ipa_get_type (callee_pi, i));
>>> +               tree type = ipa_get_type (callee_pi, i);
>>> +               tree cst = ipa_value_from_jfunc (caller_parms_info, jf, type);
>>>
>>>                  if (!cst && e->call_stmt
>>>                      && i < (int)gimple_call_num_args (e->call_stmt))
>>> @@ -659,10 +659,10 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>>                      && vrp_will_run_p (caller)
>>>                      && ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>>>                    {
>>> -                   value_range vr
>>> -                      = ipa_value_range_from_jfunc (caller_parms_info, e, jf,
>>> -                                                    ipa_get_type (callee_pi,
>>> -                                                                  i));
>>> +                   Value_Range vr (type);
>>> +
>>> +                   ipa_value_range_from_jfunc (vr, caller_parms_info, e, jf,
>>> +                                               ipa_get_type (callee_pi, i));
> 
> I guess that the ipa_get_type call can also be replaced with "type" now.

Done.

> 
>>>                      if (!vr.undefined_p () && !vr.varying_p ())
>>>                        {
>>>                          if (!avals->m_known_value_ranges.length ())
>>> @@ -670,7 +670,7 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>>                              avals->m_known_value_ranges.safe_grow (count, true);
>>>                              for (int i = 0; i < count; ++i)
>>>                                new (&avals->m_known_value_ranges[i])
>>> -                               value_range ();
>>> +                               Value_Range ();
>>>                            }
>>>                          avals->m_known_value_ranges[i] = vr;
>>>                        }
> 
> Thanks for working on this and sorry that it takes me so long to review.
> 
> Martin
> 

How's this?

Aldy

Hi,

thanks for dealing with my requests.

On Wed, Jun 07 2023, Aldy Hernandez wrote:
> On 5/26/23 18:17, Martin Jambor wrote:
>> Hello,
>> 
>> On Mon, May 22 2023, Aldy Hernandez wrote:
>>> I've adjusted the patch with some minor cleanups that came up when I
>>> implemented the rest of the IPA revamp.
>>>
>>> Rested.  OK?
>>>
>>> On Wed, May 17, 2023 at 4:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>
>>>> This converts the lattice to store ranges in Value_Range instead of
>>>> value_range (*) to make it type agnostic, and adjust all users
>>>> accordingly.
>>>>
>>>> I think it is a good example on converting from static ranges to more
>>>> general, type agnostic ones.
>>>>
>>>> I've been careful to make sure Value_Range never ends up on GC, since
>>>> it contains an int_range_max and can expand on-demand onto the heap.
>>>> Longer term storage for ranges should be done with vrange_storage, as
>>>> per the previous patch ("Provide an API for ipa_vr").
>>>>
>>>> (*) I do know the Value_Range naming versus value_range is quite
>>>> annoying, but it was a judgement call last release for the eventual
>>>> migration to having "value_range" be a type agnostic range object.  We
>>>> will ultimately rename Value_Range to value_range.

[...]

>>>> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
>>>> index d4b9d4ac27e..bd5b1da17b2 100644
>>>> --- a/gcc/ipa-cp.cc
>>>> +++ b/gcc/ipa-cp.cc
>>>> @@ -343,20 +343,29 @@ private:
>>>>   class ipcp_vr_lattice
>>>>   {
>>>>   public:
>>>> -  value_range m_vr;
>>>> +  Value_Range m_vr;
>>>>
>>>>     inline bool bottom_p () const;
>>>>     inline bool top_p () const;
>>>> -  inline bool set_to_bottom ();
>>>> -  bool meet_with (const value_range *p_vr);
>>>> +  inline bool set_to_bottom (tree type);
>> 
>> Requiring a type when setting a lattice to bottom makes for a weird
>> interface, can't we set the underlying Value_Range to whatever... >
>>>> +  bool meet_with (const vrange &p_vr);
>>>>     bool meet_with (const ipcp_vr_lattice &other);
>>>> -  void init () { gcc_assert (m_vr.undefined_p ()); }
>>>> +  void init (tree type);
>>>>     void print (FILE * f);
>>>>
>>>>   private:
>>>> -  bool meet_with_1 (const value_range *other_vr);
>>>> +  bool meet_with_1 (const vrange &other_vr);
>>>>   };
>>>>
>>>> +inline void
>>>> +ipcp_vr_lattice::init (tree type)
>>>> +{
>>>> +  if (type)
>>>> +    m_vr.set_type (type);
>>>> +
>>>> +  // Otherwise m_vr will default to unsupported_range.
>> 
>> ...this does?
>> 
>> All users of the lattice check it for not being bottom first, so it
>> should be safe.
>> 
>> If it is not possible for some reason, then I guess we should add a bool
>> flag to ipcp_vr_lattice instead, rather than looking up types of
>> unusable lattices.  ipcp_vr_lattices don't live for long.
>
> The type was my least favorite part of this work.  And yes, your 
> suggestion would work.  I have tweaked the patch to force a VARYING for 
> an unsupported range which seems to do the trick.  It looks much 
> cleaner.  Thanks.

This version is much better indeed.

[...]

>>>> @@ -1912,29 +1917,33 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr,
>>>>       return false;
>>>>
>>>>     range_op_handler handler (operation, dst_type);
>>>> -  return (handler
>>>> -         && handler.fold_range (*dst_vr, dst_type,
>>>> -                                *src_vr, value_range (dst_type))
>>>> -         && !dst_vr->varying_p ()
>>>> -         && !dst_vr->undefined_p ());
>>>> +  if (!handler)
>>>> +    return false;
>>>> +
>>>> +  Value_Range varying (dst_type);
>>>> +  varying.set_varying (dst_type);
>>>> +
>>>> +  return (handler.fold_range (dst_vr, dst_type, src_vr, varying)
>>>> +         && !dst_vr.varying_p ()
>>>> +         && !dst_vr.undefined_p ());
>>>>   }
>>>>
>>>>   /* Determine value_range of JFUNC given that INFO describes the caller node or
>>>>      the one it is inlined to, CS is the call graph edge corresponding to JFUNC
>>>>      and PARM_TYPE of the parameter.  */
>>>>
>>>> -value_range
>>>> -ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>>> +void
>>>> +ipa_value_range_from_jfunc (vrange &vr,
>>>> +                           ipa_node_params *info, cgraph_edge *cs,
>>>>                              ipa_jump_func *jfunc, tree parm_type)
>> 
>> I assume that you decided to return the value in a parameter passed by
>> reference instead of in return value for a good reason but then can we
>> at least...
>
> vrange is an abstract type, plus it can be any size (int_range<3> has 3 
> sub-ranges, legacy value_range has 2 sub-ranges, frange is a totally 
> different object, etc).  Throughout all of ranger, returning a range is 
> done by passing by reference.  This has the added benefit that sometimes 
> we can set a return range by twiddling a few bits (foo.set_undefined()) 
> instead of having to copy a full range back and forth.
>

I see, thanks.

[...]

>
> How's this?

One minor observation below...

>
> Aldy
> From 2fd0ae47aa094675a02763e72d7bb7404ed9334b Mon Sep 17 00:00:00 2001
> From: Aldy Hernandez <aldyh@redhat.com>
> Date: Wed, 17 May 2023 11:29:34 +0200
> Subject: [PATCH] Convert ipcp_vr_lattice to type agnostic framework.
>
> This converts the lattice to store ranges in Value_Range instead of
> value_range (*) to make it type agnostic, and adjust all users
> accordingly.
>
> I've been careful to make sure Value_Range never ends up on GC, since
> it contains an int_range_max and can expand on-demand onto the heap.
> Longer term storage for ranges should be done with vrange_storage, as
> per the previous patch ("Provide an API for ipa_vr").
>
> gcc/ChangeLog:
>
> 	* ipa-cp.cc (ipcp_vr_lattice::init): Take type argument.
> 	(ipcp_vr_lattice::print): Call dump method.
> 	(ipcp_vr_lattice::meet_with): Adjust for m_vr being a
>         Value_Range.
> 	(ipcp_vr_lattice::meet_with_1): Make argument a reference.
> 	(ipcp_vr_lattice::set_to_bottom): Set varying for an unsupported
> 	range.
> 	(initialize_node_lattices): Pass type when appropriate.
> 	(ipa_vr_operation_and_type_effects): Make type agnostic.
> 	(ipa_value_range_from_jfunc): Same.
> 	(propagate_vr_across_jump_function): Same.
> 	* ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same.
> 	(evaluate_properties_for_edge): Same.
> 	* ipa-prop.cc (ipa_vr::get_vrange): Same.
> 	(ipcp_update_vr): Same.
> 	* ipa-prop.h (ipa_value_range_from_jfunc): Same.
> 	(ipa_range_set_and_normalize): Same.
> ---
>  gcc/ipa-cp.cc        | 148 ++++++++++++++++++++++++-------------------
>  gcc/ipa-fnsummary.cc |  15 ++---
>  gcc/ipa-prop.cc      |   5 +-
>  gcc/ipa-prop.h       |  21 +++---
>  4 files changed, 101 insertions(+), 88 deletions(-)
>
> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
> index 0f37bb5e336..d77b9eab249 100644
> --- a/gcc/ipa-cp.cc
> +++ b/gcc/ipa-cp.cc
> @@ -343,20 +343,29 @@ private:
>  class ipcp_vr_lattice
>  {
>  public:
> -  value_range m_vr;
> +  Value_Range m_vr;
>  
>    inline bool bottom_p () const;
>    inline bool top_p () const;
>    inline bool set_to_bottom ();
> -  bool meet_with (const value_range *p_vr);
> +  bool meet_with (const vrange &p_vr);
>    bool meet_with (const ipcp_vr_lattice &other);
> -  void init () { gcc_assert (m_vr.undefined_p ()); }
> +  void init (tree type);
>    void print (FILE * f);
>  
>  private:
> -  bool meet_with_1 (const value_range *other_vr);
> +  bool meet_with_1 (const vrange &other_vr);
>  };
>  
> +inline void
> +ipcp_vr_lattice::init (tree type)
> +{
> +  if (type)
> +    m_vr.set_type (type);
> +
> +  // Otherwise m_vr will default to unsupported_range.
> +}
> +
>  /* Structure containing lattices for a parameter itself and for pieces of
>     aggregates that are passed in the parameter or by a reference in a parameter
>     plus some other useful flags.  */
> @@ -585,7 +594,7 @@ ipcp_bits_lattice::print (FILE *f)
>  void
>  ipcp_vr_lattice::print (FILE * f)
>  {
> -  dump_value_range (f, &m_vr);
> +  m_vr.dump (f);
>  }
>  
>  /* Print all ipcp_lattices of all functions to F.  */
> @@ -1016,39 +1025,39 @@ set_agg_lats_contain_variable (class ipcp_param_lattices *plats)
>  bool
>  ipcp_vr_lattice::meet_with (const ipcp_vr_lattice &other)
>  {
> -  return meet_with_1 (&other.m_vr);
> +  return meet_with_1 (other.m_vr);
>  }
>  
> -/* Meet the current value of the lattice with value range described by VR
> -   lattice.  */
> +/* Meet the current value of the lattice with the range described by
> +   P_VR.  */
>  
>  bool
> -ipcp_vr_lattice::meet_with (const value_range *p_vr)
> +ipcp_vr_lattice::meet_with (const vrange &p_vr)
>  {
>    return meet_with_1 (p_vr);
>  }
>  
> -/* Meet the current value of the lattice with value range described by
> -   OTHER_VR lattice.  Return TRUE if anything changed.  */
> +/* Meet the current value of the lattice with the range described by
> +   OTHER_VR.  Return TRUE if anything changed.  */
>  
>  bool
> -ipcp_vr_lattice::meet_with_1 (const value_range *other_vr)
> +ipcp_vr_lattice::meet_with_1 (const vrange &other_vr)
>  {
>    if (bottom_p ())
>      return false;
>  
> -  if (other_vr->varying_p ())
> +  if (other_vr.varying_p ())
>      return set_to_bottom ();
>  
>    bool res;
>    if (flag_checking)
>      {
> -      value_range save (m_vr);
> -      res = m_vr.union_ (*other_vr);
> +      Value_Range save (m_vr);
> +      res = m_vr.union_ (other_vr);
>        gcc_assert (res == (m_vr != save));
>      }
>    else
> -    res = m_vr.union_ (*other_vr);
> +    res = m_vr.union_ (other_vr);
>    return res;
>  }
>  
> @@ -1077,12 +1086,15 @@ ipcp_vr_lattice::set_to_bottom ()
>  {
>    if (m_vr.varying_p ())
>      return false;
> -  /* ?? We create all sorts of VARYING ranges for floats, structures,
> -     and other types which we cannot handle as ranges.  We should
> -     probably avoid handling them throughout the pass, but it's easier
> -     to create a sensible VARYING here and let the lattice
> -     propagate.  */
> -  m_vr.set_varying (integer_type_node);
> +
> +  /* Setting an unsupported type here forces the temporary to default
> +     to unsupported_range, which can handle VARYING/DEFINED ranges,
> +     but nothing else (union, intersect, etc).  This allows us to set
> +     bottoms on any ranges, and is safe as all users of the lattice
> +     check for bottom first.  */
> +  m_vr.set_type (void_type_node);
> +  m_vr.set_varying (void_type_node);
> +
>    return true;
>  }
>  
> @@ -1653,6 +1665,7 @@ initialize_node_lattices (struct cgraph_node *node)
>    for (i = 0; i < ipa_get_param_count (info); i++)
>      {
>        ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
> +      tree type = ipa_get_type (info, i);
>        if (disable
>  	  || !ipa_get_type (info, i)
>  	  || (pre_modified && (surviving_params.length () <= (unsigned) i
> @@ -1662,12 +1675,12 @@ initialize_node_lattices (struct cgraph_node *node)
>  	  plats->ctxlat.set_to_bottom ();
>  	  set_agg_lats_to_bottom (plats);
>  	  plats->bits_lattice.set_to_bottom ();
> -	  plats->m_value_range.m_vr = value_range ();
> +	  plats->m_value_range.init (type);
>  	  plats->m_value_range.set_to_bottom ();

This sequence of init(type) followed by set_to_bottom looks a little
superfluous, is it?

>  	}
>        else
>  	{
> -	  plats->m_value_range.init ();
> +	  plats->m_value_range.init (type);

Same here.

But the patch is OK either way.

Thanks again,

Martin


>  	  if (variable)
>  	    set_all_contains_variable (plats);
>  	}
> @@ -1900,11 +1913,11 @@ ipa_context_from_jfunc (ipa_node_params *info, cgraph_edge *cs, int csidx,
>  
>  /* Emulate effects of unary OPERATION and/or conversion from SRC_TYPE to
>     DST_TYPE on value range in SRC_VR and store it to DST_VR.  Return true if
> -   the result is a range or an anti-range.  */
> +   the result is a range that is not VARYING nor UNDEFINED.  */
>  
>  static bool
> -ipa_vr_operation_and_type_effects (value_range *dst_vr,
> -				   value_range *src_vr,
> +ipa_vr_operation_and_type_effects (vrange &dst_vr,
> +				   const vrange &src_vr,
>  				   enum tree_code operation,
>  				   tree dst_type, tree src_type)
>  {
> @@ -1912,29 +1925,35 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr,
>      return false;
>  
>    range_op_handler handler (operation, dst_type);
> -  return (handler
> -	  && handler.fold_range (*dst_vr, dst_type,
> -				 *src_vr, value_range (dst_type))
> -	  && !dst_vr->varying_p ()
> -	  && !dst_vr->undefined_p ());
> +  if (!handler)
> +    return false;
> +
> +  Value_Range varying (dst_type);
> +  varying.set_varying (dst_type);
> +
> +  return (handler.fold_range (dst_vr, dst_type, src_vr, varying)
> +	  && !dst_vr.varying_p ()
> +	  && !dst_vr.undefined_p ());
>  }
>  
>  /* Determine range of JFUNC given that INFO describes the caller node or
>     the one it is inlined to, CS is the call graph edge corresponding to JFUNC
>     and PARM_TYPE of the parameter.  */
>  
> -value_range
> -ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
> +void
> +ipa_value_range_from_jfunc (vrange &vr,
> +			    ipa_node_params *info, cgraph_edge *cs,
>  			    ipa_jump_func *jfunc, tree parm_type)
>  {
> -  value_range vr;
> +  vr.set_undefined ();
> +
>    if (jfunc->m_vr)
> -    ipa_vr_operation_and_type_effects (&vr,
> -				       jfunc->m_vr,
> +    ipa_vr_operation_and_type_effects (vr,
> +				       *jfunc->m_vr,
>  				       NOP_EXPR, parm_type,
>  				       jfunc->m_vr->type ());
>    if (vr.singleton_p ())
> -    return vr;
> +    return;
>    if (jfunc->type == IPA_JF_PASS_THROUGH)
>      {
>        int idx;
> @@ -1943,33 +1962,34 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>  					   ? cs->caller->inlined_to
>  					   : cs->caller);
>        if (!sum || !sum->m_vr)
> -	return vr;
> +	return;
>  
>        idx = ipa_get_jf_pass_through_formal_id (jfunc);
>  
>        if (!(*sum->m_vr)[idx].known_p ())
> -	return vr;
> +	return;
>        tree vr_type = ipa_get_type (info, idx);
> -      value_range srcvr;
> +      Value_Range srcvr;
>        (*sum->m_vr)[idx].get_vrange (srcvr);
>  
>        enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>  
>        if (TREE_CODE_CLASS (operation) == tcc_unary)
>  	{
> -	  value_range res;
> +	  Value_Range res (vr_type);
>  
> -	  if (ipa_vr_operation_and_type_effects (&res,
> -						 &srcvr,
> +	  if (ipa_vr_operation_and_type_effects (res,
> +						 srcvr,
>  						 operation, parm_type,
>  						 vr_type))
>  	    vr.intersect (res);
>  	}
>        else
>  	{
> -	  value_range op_res, res;
> +	  Value_Range op_res (vr_type);
> +	  Value_Range res (vr_type);
>  	  tree op = ipa_get_jf_pass_through_operand (jfunc);
> -	  value_range op_vr;
> +	  Value_Range op_vr (vr_type);
>  	  range_op_handler handler (operation, vr_type);
>  
>  	  ipa_range_set_and_normalize (op_vr, op);
> @@ -1979,14 +1999,13 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>  	      || !handler.fold_range (op_res, vr_type, srcvr, op_vr))
>  	    op_res.set_varying (vr_type);
>  
> -	  if (ipa_vr_operation_and_type_effects (&res,
> -						 &op_res,
> +	  if (ipa_vr_operation_and_type_effects (res,
> +						 op_res,
>  						 NOP_EXPR, parm_type,
>  						 vr_type))
>  	    vr.intersect (res);
>  	}
>      }
> -  return vr;
>  }
>  
>  /* Determine whether ITEM, jump function for an aggregate part, evaluates to a
> @@ -2753,10 +2772,10 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>        if (src_lats->m_value_range.bottom_p ())
>  	return dest_lat->set_to_bottom ();
>  
> -      value_range vr;
> +      Value_Range vr (operand_type);
>        if (TREE_CODE_CLASS (operation) == tcc_unary)
> -	ipa_vr_operation_and_type_effects (&vr,
> -					   &src_lats->m_value_range.m_vr,
> +	ipa_vr_operation_and_type_effects (vr,
> +					   src_lats->m_value_range.m_vr,
>  					   operation, param_type,
>  					   operand_type);
>        /* A crude way to prevent unbounded number of value range updates
> @@ -2765,8 +2784,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>        else if (!ipa_edge_within_scc (cs))
>  	{
>  	  tree op = ipa_get_jf_pass_through_operand (jfunc);
> -	  value_range op_vr;
> -	  value_range op_res,res;
> +	  Value_Range op_vr (TREE_TYPE (op));
> +	  Value_Range op_res (operand_type);
>  	  range_op_handler handler (operation, operand_type);
>  
>  	  ipa_range_set_and_normalize (op_vr, op);
> @@ -2777,8 +2796,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>  				      src_lats->m_value_range.m_vr, op_vr))
>  	    op_res.set_varying (operand_type);
>  
> -	  ipa_vr_operation_and_type_effects (&vr,
> -					     &op_res,
> +	  ipa_vr_operation_and_type_effects (vr,
> +					     op_res,
>  					     NOP_EXPR, param_type,
>  					     operand_type);
>  	}
> @@ -2786,14 +2805,14 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>  	{
>  	  if (jfunc->m_vr)
>  	    {
> -	      value_range jvr;
> -	      if (ipa_vr_operation_and_type_effects (&jvr, jfunc->m_vr,
> +	      Value_Range jvr (param_type);
> +	      if (ipa_vr_operation_and_type_effects (jvr, *jfunc->m_vr,
>  						     NOP_EXPR,
>  						     param_type,
>  						     jfunc->m_vr->type ()))
>  		vr.intersect (jvr);
>  	    }
> -	  return dest_lat->meet_with (&vr);
> +	  return dest_lat->meet_with (vr);
>  	}
>      }
>    else if (jfunc->type == IPA_JF_CONST)
> @@ -2805,18 +2824,17 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>  	  if (TREE_OVERFLOW_P (val))
>  	    val = drop_tree_overflow (val);
>  
> -	  value_range tmpvr (TREE_TYPE (val),
> -			     wi::to_wide (val), wi::to_wide (val));
> -	  return dest_lat->meet_with (&tmpvr);
> +	  Value_Range tmpvr (val, val);
> +	  return dest_lat->meet_with (tmpvr);
>  	}
>      }
>  
> -  value_range vr;
> +  Value_Range vr (param_type);
>    if (jfunc->m_vr
> -      && ipa_vr_operation_and_type_effects (&vr, jfunc->m_vr, NOP_EXPR,
> +      && ipa_vr_operation_and_type_effects (vr, *jfunc->m_vr, NOP_EXPR,
>  					    param_type,
>  					    jfunc->m_vr->type ()))
> -    return dest_lat->meet_with (&vr);
> +    return dest_lat->meet_with (vr);
>    else
>      return dest_lat->set_to_bottom ();
>  }
> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> index b328bb8ce14..cf416202920 100644
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> @@ -475,7 +475,7 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
>  	  && !c->agg_contents
>  	  && (!val || TREE_CODE (val) != INTEGER_CST))
>  	{
> -	  value_range vr = avals->m_known_value_ranges[c->operand_num];
> +	  Value_Range vr (avals->m_known_value_ranges[c->operand_num]);
>  	  if (!vr.undefined_p ()
>  	      && !vr.varying_p ()
>  	      && (TYPE_SIZE (c->type) == TYPE_SIZE (vr.type ())))
> @@ -630,8 +630,8 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>  		|| ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>  	      {
>  		/* Determine if we know constant value of the parameter.  */
> -		tree cst = ipa_value_from_jfunc (caller_parms_info, jf,
> -						 ipa_get_type (callee_pi, i));
> +		tree type = ipa_get_type (callee_pi, i);
> +		tree cst = ipa_value_from_jfunc (caller_parms_info, jf, type);
>  
>  		if (!cst && e->call_stmt
>  		    && i < (int)gimple_call_num_args (e->call_stmt))
> @@ -659,10 +659,9 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>  		    && vrp_will_run_p (caller)
>  		    && ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>  		  {
> -		    value_range vr
> -		       = ipa_value_range_from_jfunc (caller_parms_info, e, jf,
> -						     ipa_get_type (callee_pi,
> -								   i));
> +		    Value_Range vr (type);
> +
> +		    ipa_value_range_from_jfunc (vr, caller_parms_info, e, jf, type);
>  		    if (!vr.undefined_p () && !vr.varying_p ())
>  		      {
>  			if (!avals->m_known_value_ranges.length ())
> @@ -670,7 +669,7 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>  			    avals->m_known_value_ranges.safe_grow (count, true);
>  			    for (int i = 0; i < count; ++i)
>  			      new (&avals->m_known_value_ranges[i])
> -				value_range ();
> +				Value_Range ();
>  			  }
>  			avals->m_known_value_ranges[i] = vr;
>  		      }
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index ab6de9f10da..bbfe0f8aa45 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -198,8 +198,9 @@ ipa_vr::equal_p (const vrange &r) const
>  }
>  
>  void
> -ipa_vr::get_vrange (vrange &r) const
> +ipa_vr::get_vrange (Value_Range &r) const
>  {
> +  r.set_type (m_type);
>    m_storage->get_vrange (r, m_type);
>  }
>  
> @@ -5963,7 +5964,7 @@ ipcp_update_vr (struct cgraph_node *node)
>  
>        if (vr[i].known_p ())
>  	{
> -	  value_range tmp;
> +	  Value_Range tmp;
>  	  vr[i].get_vrange (tmp);
>  
>  	  if (!tmp.undefined_p () && !tmp.varying_p ())
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index f306f8a377e..3a591a8f44d 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -314,7 +314,7 @@ public:
>    void set_unknown ();
>    bool known_p () const { return m_storage != NULL; }
>    tree type () const { return m_type; }
> -  void get_vrange (vrange &) const;
> +  void get_vrange (Value_Range &) const;
>    bool equal_p (const vrange &) const;
>    const vrange_storage *storage () const { return m_storage; }
>    void streamer_read (lto_input_block *, data_in *);
> @@ -530,7 +530,7 @@ public:
>    auto_vec<ipa_argagg_value, 32> m_known_aggs;
>  
>    /* Vector describing known value ranges of arguments.  */
> -  auto_vec<value_range, 32> m_known_value_ranges;
> +  auto_vec<Value_Range, 32> m_known_value_ranges;
>  };
>  
>  inline
> @@ -582,7 +582,7 @@ public:
>    vec<ipa_argagg_value> m_known_aggs = vNULL;
>  
>    /* Vector describing known value ranges of arguments.  */
> -  vec<value_range> m_known_value_ranges = vNULL;
> +  vec<Value_Range> m_known_value_ranges = vNULL;
>  };
>  
>  inline
> @@ -1194,8 +1194,8 @@ ipa_polymorphic_call_context ipa_context_from_jfunc (ipa_node_params *,
>  						     cgraph_edge *,
>  						     int,
>  						     ipa_jump_func *);
> -value_range ipa_value_range_from_jfunc (ipa_node_params *, cgraph_edge *,
> -					ipa_jump_func *, tree);
> +void ipa_value_range_from_jfunc (vrange &, ipa_node_params *, cgraph_edge *,
> +				 ipa_jump_func *, tree);
>  void ipa_push_agg_values_from_jfunc (ipa_node_params *info, cgraph_node *node,
>  				     ipa_agg_jump_function *agg_jfunc,
>  				     unsigned dst_index,
> @@ -1218,17 +1218,12 @@ void ipa_cp_cc_finalize (void);
>     non-zero.  */
>  
>  inline void
> -ipa_range_set_and_normalize (irange &r, tree val)
> +ipa_range_set_and_normalize (vrange &r, tree val)
>  {
> -  if (TREE_CODE (val) == INTEGER_CST)
> -    {
> -      wide_int w = wi::to_wide (val);
> -      r.set (TREE_TYPE (val), w, w);
> -    }
> -  else if (TREE_CODE (val) == ADDR_EXPR)
> +  if (TREE_CODE (val) == ADDR_EXPR)
>      r.set_nonzero (TREE_TYPE (val));
>    else
> -    r.set_varying (TREE_TYPE (val));
> +    r.set (val, val);
>  }
>  
>  #endif /* IPA_PROP_H */
> -- 
> 2.40.1

On 6/10/23 10:49, Martin Jambor wrote:
> Hi,
> 
> thanks for dealing with my requests.
> 
> On Wed, Jun 07 2023, Aldy Hernandez wrote:
>> On 5/26/23 18:17, Martin Jambor wrote:
>>> Hello,
>>>
>>> On Mon, May 22 2023, Aldy Hernandez wrote:
>>>> I've adjusted the patch with some minor cleanups that came up when I
>>>> implemented the rest of the IPA revamp.
>>>>
>>>> Rested.  OK?
>>>>
>>>> On Wed, May 17, 2023 at 4:31 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>>>
>>>>> This converts the lattice to store ranges in Value_Range instead of
>>>>> value_range (*) to make it type agnostic, and adjust all users
>>>>> accordingly.
>>>>>
>>>>> I think it is a good example on converting from static ranges to more
>>>>> general, type agnostic ones.
>>>>>
>>>>> I've been careful to make sure Value_Range never ends up on GC, since
>>>>> it contains an int_range_max and can expand on-demand onto the heap.
>>>>> Longer term storage for ranges should be done with vrange_storage, as
>>>>> per the previous patch ("Provide an API for ipa_vr").
>>>>>
>>>>> (*) I do know the Value_Range naming versus value_range is quite
>>>>> annoying, but it was a judgement call last release for the eventual
>>>>> migration to having "value_range" be a type agnostic range object.  We
>>>>> will ultimately rename Value_Range to value_range.
> 
> [...]
> 
>>>>> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
>>>>> index d4b9d4ac27e..bd5b1da17b2 100644
>>>>> --- a/gcc/ipa-cp.cc
>>>>> +++ b/gcc/ipa-cp.cc
>>>>> @@ -343,20 +343,29 @@ private:
>>>>>    class ipcp_vr_lattice
>>>>>    {
>>>>>    public:
>>>>> -  value_range m_vr;
>>>>> +  Value_Range m_vr;
>>>>>
>>>>>      inline bool bottom_p () const;
>>>>>      inline bool top_p () const;
>>>>> -  inline bool set_to_bottom ();
>>>>> -  bool meet_with (const value_range *p_vr);
>>>>> +  inline bool set_to_bottom (tree type);
>>>
>>> Requiring a type when setting a lattice to bottom makes for a weird
>>> interface, can't we set the underlying Value_Range to whatever... >
>>>>> +  bool meet_with (const vrange &p_vr);
>>>>>      bool meet_with (const ipcp_vr_lattice &other);
>>>>> -  void init () { gcc_assert (m_vr.undefined_p ()); }
>>>>> +  void init (tree type);
>>>>>      void print (FILE * f);
>>>>>
>>>>>    private:
>>>>> -  bool meet_with_1 (const value_range *other_vr);
>>>>> +  bool meet_with_1 (const vrange &other_vr);
>>>>>    };
>>>>>
>>>>> +inline void
>>>>> +ipcp_vr_lattice::init (tree type)
>>>>> +{
>>>>> +  if (type)
>>>>> +    m_vr.set_type (type);
>>>>> +
>>>>> +  // Otherwise m_vr will default to unsupported_range.
>>>
>>> ...this does?
>>>
>>> All users of the lattice check it for not being bottom first, so it
>>> should be safe.
>>>
>>> If it is not possible for some reason, then I guess we should add a bool
>>> flag to ipcp_vr_lattice instead, rather than looking up types of
>>> unusable lattices.  ipcp_vr_lattices don't live for long.
>>
>> The type was my least favorite part of this work.  And yes, your
>> suggestion would work.  I have tweaked the patch to force a VARYING for
>> an unsupported range which seems to do the trick.  It looks much
>> cleaner.  Thanks.
> 
> This version is much better indeed.
> 
> [...]
> 
>>>>> @@ -1912,29 +1917,33 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr,
>>>>>        return false;
>>>>>
>>>>>      range_op_handler handler (operation, dst_type);
>>>>> -  return (handler
>>>>> -         && handler.fold_range (*dst_vr, dst_type,
>>>>> -                                *src_vr, value_range (dst_type))
>>>>> -         && !dst_vr->varying_p ()
>>>>> -         && !dst_vr->undefined_p ());
>>>>> +  if (!handler)
>>>>> +    return false;
>>>>> +
>>>>> +  Value_Range varying (dst_type);
>>>>> +  varying.set_varying (dst_type);
>>>>> +
>>>>> +  return (handler.fold_range (dst_vr, dst_type, src_vr, varying)
>>>>> +         && !dst_vr.varying_p ()
>>>>> +         && !dst_vr.undefined_p ());
>>>>>    }
>>>>>
>>>>>    /* Determine value_range of JFUNC given that INFO describes the caller node or
>>>>>       the one it is inlined to, CS is the call graph edge corresponding to JFUNC
>>>>>       and PARM_TYPE of the parameter.  */
>>>>>
>>>>> -value_range
>>>>> -ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>>>> +void
>>>>> +ipa_value_range_from_jfunc (vrange &vr,
>>>>> +                           ipa_node_params *info, cgraph_edge *cs,
>>>>>                               ipa_jump_func *jfunc, tree parm_type)
>>>
>>> I assume that you decided to return the value in a parameter passed by
>>> reference instead of in return value for a good reason but then can we
>>> at least...
>>
>> vrange is an abstract type, plus it can be any size (int_range<3> has 3
>> sub-ranges, legacy value_range has 2 sub-ranges, frange is a totally
>> different object, etc).  Throughout all of ranger, returning a range is
>> done by passing by reference.  This has the added benefit that sometimes
>> we can set a return range by twiddling a few bits (foo.set_undefined())
>> instead of having to copy a full range back and forth.
>>
> 
> I see, thanks.
> 
> [...]
> 
>>
>> How's this?
> 
> One minor observation below...
> 
>>
>> Aldy
>>  From 2fd0ae47aa094675a02763e72d7bb7404ed9334b Mon Sep 17 00:00:00 2001
>> From: Aldy Hernandez <aldyh@redhat.com>
>> Date: Wed, 17 May 2023 11:29:34 +0200
>> Subject: [PATCH] Convert ipcp_vr_lattice to type agnostic framework.
>>
>> This converts the lattice to store ranges in Value_Range instead of
>> value_range (*) to make it type agnostic, and adjust all users
>> accordingly.
>>
>> I've been careful to make sure Value_Range never ends up on GC, since
>> it contains an int_range_max and can expand on-demand onto the heap.
>> Longer term storage for ranges should be done with vrange_storage, as
>> per the previous patch ("Provide an API for ipa_vr").
>>
>> gcc/ChangeLog:
>>
>> 	* ipa-cp.cc (ipcp_vr_lattice::init): Take type argument.
>> 	(ipcp_vr_lattice::print): Call dump method.
>> 	(ipcp_vr_lattice::meet_with): Adjust for m_vr being a
>>          Value_Range.
>> 	(ipcp_vr_lattice::meet_with_1): Make argument a reference.
>> 	(ipcp_vr_lattice::set_to_bottom): Set varying for an unsupported
>> 	range.
>> 	(initialize_node_lattices): Pass type when appropriate.
>> 	(ipa_vr_operation_and_type_effects): Make type agnostic.
>> 	(ipa_value_range_from_jfunc): Same.
>> 	(propagate_vr_across_jump_function): Same.
>> 	* ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same.
>> 	(evaluate_properties_for_edge): Same.
>> 	* ipa-prop.cc (ipa_vr::get_vrange): Same.
>> 	(ipcp_update_vr): Same.
>> 	* ipa-prop.h (ipa_value_range_from_jfunc): Same.
>> 	(ipa_range_set_and_normalize): Same.
>> ---
>>   gcc/ipa-cp.cc        | 148 ++++++++++++++++++++++++-------------------
>>   gcc/ipa-fnsummary.cc |  15 ++---
>>   gcc/ipa-prop.cc      |   5 +-
>>   gcc/ipa-prop.h       |  21 +++---
>>   4 files changed, 101 insertions(+), 88 deletions(-)
>>
>> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
>> index 0f37bb5e336..d77b9eab249 100644
>> --- a/gcc/ipa-cp.cc
>> +++ b/gcc/ipa-cp.cc
>> @@ -343,20 +343,29 @@ private:
>>   class ipcp_vr_lattice
>>   {
>>   public:
>> -  value_range m_vr;
>> +  Value_Range m_vr;
>>   
>>     inline bool bottom_p () const;
>>     inline bool top_p () const;
>>     inline bool set_to_bottom ();
>> -  bool meet_with (const value_range *p_vr);
>> +  bool meet_with (const vrange &p_vr);
>>     bool meet_with (const ipcp_vr_lattice &other);
>> -  void init () { gcc_assert (m_vr.undefined_p ()); }
>> +  void init (tree type);
>>     void print (FILE * f);
>>   
>>   private:
>> -  bool meet_with_1 (const value_range *other_vr);
>> +  bool meet_with_1 (const vrange &other_vr);
>>   };
>>   
>> +inline void
>> +ipcp_vr_lattice::init (tree type)
>> +{
>> +  if (type)
>> +    m_vr.set_type (type);
>> +
>> +  // Otherwise m_vr will default to unsupported_range.
>> +}
>> +
>>   /* Structure containing lattices for a parameter itself and for pieces of
>>      aggregates that are passed in the parameter or by a reference in a parameter
>>      plus some other useful flags.  */
>> @@ -585,7 +594,7 @@ ipcp_bits_lattice::print (FILE *f)
>>   void
>>   ipcp_vr_lattice::print (FILE * f)
>>   {
>> -  dump_value_range (f, &m_vr);
>> +  m_vr.dump (f);
>>   }
>>   
>>   /* Print all ipcp_lattices of all functions to F.  */
>> @@ -1016,39 +1025,39 @@ set_agg_lats_contain_variable (class ipcp_param_lattices *plats)
>>   bool
>>   ipcp_vr_lattice::meet_with (const ipcp_vr_lattice &other)
>>   {
>> -  return meet_with_1 (&other.m_vr);
>> +  return meet_with_1 (other.m_vr);
>>   }
>>   
>> -/* Meet the current value of the lattice with value range described by VR
>> -   lattice.  */
>> +/* Meet the current value of the lattice with the range described by
>> +   P_VR.  */
>>   
>>   bool
>> -ipcp_vr_lattice::meet_with (const value_range *p_vr)
>> +ipcp_vr_lattice::meet_with (const vrange &p_vr)
>>   {
>>     return meet_with_1 (p_vr);
>>   }
>>   
>> -/* Meet the current value of the lattice with value range described by
>> -   OTHER_VR lattice.  Return TRUE if anything changed.  */
>> +/* Meet the current value of the lattice with the range described by
>> +   OTHER_VR.  Return TRUE if anything changed.  */
>>   
>>   bool
>> -ipcp_vr_lattice::meet_with_1 (const value_range *other_vr)
>> +ipcp_vr_lattice::meet_with_1 (const vrange &other_vr)
>>   {
>>     if (bottom_p ())
>>       return false;
>>   
>> -  if (other_vr->varying_p ())
>> +  if (other_vr.varying_p ())
>>       return set_to_bottom ();
>>   
>>     bool res;
>>     if (flag_checking)
>>       {
>> -      value_range save (m_vr);
>> -      res = m_vr.union_ (*other_vr);
>> +      Value_Range save (m_vr);
>> +      res = m_vr.union_ (other_vr);
>>         gcc_assert (res == (m_vr != save));
>>       }
>>     else
>> -    res = m_vr.union_ (*other_vr);
>> +    res = m_vr.union_ (other_vr);
>>     return res;
>>   }
>>   
>> @@ -1077,12 +1086,15 @@ ipcp_vr_lattice::set_to_bottom ()
>>   {
>>     if (m_vr.varying_p ())
>>       return false;
>> -  /* ?? We create all sorts of VARYING ranges for floats, structures,
>> -     and other types which we cannot handle as ranges.  We should
>> -     probably avoid handling them throughout the pass, but it's easier
>> -     to create a sensible VARYING here and let the lattice
>> -     propagate.  */
>> -  m_vr.set_varying (integer_type_node);
>> +
>> +  /* Setting an unsupported type here forces the temporary to default
>> +     to unsupported_range, which can handle VARYING/DEFINED ranges,
>> +     but nothing else (union, intersect, etc).  This allows us to set
>> +     bottoms on any ranges, and is safe as all users of the lattice
>> +     check for bottom first.  */
>> +  m_vr.set_type (void_type_node);
>> +  m_vr.set_varying (void_type_node);
>> +
>>     return true;
>>   }
>>   
>> @@ -1653,6 +1665,7 @@ initialize_node_lattices (struct cgraph_node *node)
>>     for (i = 0; i < ipa_get_param_count (info); i++)
>>       {
>>         ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
>> +      tree type = ipa_get_type (info, i);
>>         if (disable
>>   	  || !ipa_get_type (info, i)
>>   	  || (pre_modified && (surviving_params.length () <= (unsigned) i
>> @@ -1662,12 +1675,12 @@ initialize_node_lattices (struct cgraph_node *node)
>>   	  plats->ctxlat.set_to_bottom ();
>>   	  set_agg_lats_to_bottom (plats);
>>   	  plats->bits_lattice.set_to_bottom ();
>> -	  plats->m_value_range.m_vr = value_range ();
>> +	  plats->m_value_range.init (type);
>>   	  plats->m_value_range.set_to_bottom ();
> 
> This sequence of init(type) followed by set_to_bottom looks a little
> superfluous, is it?

init() will *unconditionally* set the range to undefined, and then 
set_to_bottom will *conditionally* set the range to varying if it's not 
already varying.  This was existing behavior.

If we remove the init(), the set_to_bottom may bail on entry if the 
range was already varying (of another type):


bool
ipcp_vr_lattice::set_to_bottom ()
{
   if (m_vr.varying_p ())
     return false;
...
...

I suppose if you're checking for varying/bottom before using the 
lattice, a varying of an unsupported range is the same as a varying of 
another type, and thus excluded from calculations (??).

I left the init there to avoid reading from m_vr with it uninitialized, 
or something, but I have no idea.

> 
>>   	}
>>         else
>>   	{
>> -	  plats->m_value_range.init ();
>> +	  plats->m_value_range.init (type);
> 
> Same here.

This one is different, because the entire context is:

	{
	  plats->m_value_range.init (type);
	  if (variable)
	    set_all_contains_variable (plats);
	}

So the init() sets undefined, whereas the conditional 
set_all_contains_variable() will call set_to_bottom which will set 
varying.  So that seems to change existing behavior.

I'm gonna retest (after Andrew's changes in this area), and commit as 
is, because it seems like the safe thing to do.  If you still think we 
should delete the first init() followed by set_to_bottom(), I'm more 
than happy to do it as a follow-up.

Aldy

> 
> But the patch is OK either way.
> 
> Thanks again,
> 
> Martin
> 
> 
>>   	  if (variable)
>>   	    set_all_contains_variable (plats);
>>   	}
>> @@ -1900,11 +1913,11 @@ ipa_context_from_jfunc (ipa_node_params *info, cgraph_edge *cs, int csidx,
>>   
>>   /* Emulate effects of unary OPERATION and/or conversion from SRC_TYPE to
>>      DST_TYPE on value range in SRC_VR and store it to DST_VR.  Return true if
>> -   the result is a range or an anti-range.  */
>> +   the result is a range that is not VARYING nor UNDEFINED.  */
>>   
>>   static bool
>> -ipa_vr_operation_and_type_effects (value_range *dst_vr,
>> -				   value_range *src_vr,
>> +ipa_vr_operation_and_type_effects (vrange &dst_vr,
>> +				   const vrange &src_vr,
>>   				   enum tree_code operation,
>>   				   tree dst_type, tree src_type)
>>   {
>> @@ -1912,29 +1925,35 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr,
>>       return false;
>>   
>>     range_op_handler handler (operation, dst_type);
>> -  return (handler
>> -	  && handler.fold_range (*dst_vr, dst_type,
>> -				 *src_vr, value_range (dst_type))
>> -	  && !dst_vr->varying_p ()
>> -	  && !dst_vr->undefined_p ());
>> +  if (!handler)
>> +    return false;
>> +
>> +  Value_Range varying (dst_type);
>> +  varying.set_varying (dst_type);
>> +
>> +  return (handler.fold_range (dst_vr, dst_type, src_vr, varying)
>> +	  && !dst_vr.varying_p ()
>> +	  && !dst_vr.undefined_p ());
>>   }
>>   
>>   /* Determine range of JFUNC given that INFO describes the caller node or
>>      the one it is inlined to, CS is the call graph edge corresponding to JFUNC
>>      and PARM_TYPE of the parameter.  */
>>   
>> -value_range
>> -ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>> +void
>> +ipa_value_range_from_jfunc (vrange &vr,
>> +			    ipa_node_params *info, cgraph_edge *cs,
>>   			    ipa_jump_func *jfunc, tree parm_type)
>>   {
>> -  value_range vr;
>> +  vr.set_undefined ();
>> +
>>     if (jfunc->m_vr)
>> -    ipa_vr_operation_and_type_effects (&vr,
>> -				       jfunc->m_vr,
>> +    ipa_vr_operation_and_type_effects (vr,
>> +				       *jfunc->m_vr,
>>   				       NOP_EXPR, parm_type,
>>   				       jfunc->m_vr->type ());
>>     if (vr.singleton_p ())
>> -    return vr;
>> +    return;
>>     if (jfunc->type == IPA_JF_PASS_THROUGH)
>>       {
>>         int idx;
>> @@ -1943,33 +1962,34 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>   					   ? cs->caller->inlined_to
>>   					   : cs->caller);
>>         if (!sum || !sum->m_vr)
>> -	return vr;
>> +	return;
>>   
>>         idx = ipa_get_jf_pass_through_formal_id (jfunc);
>>   
>>         if (!(*sum->m_vr)[idx].known_p ())
>> -	return vr;
>> +	return;
>>         tree vr_type = ipa_get_type (info, idx);
>> -      value_range srcvr;
>> +      Value_Range srcvr;
>>         (*sum->m_vr)[idx].get_vrange (srcvr);
>>   
>>         enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>>   
>>         if (TREE_CODE_CLASS (operation) == tcc_unary)
>>   	{
>> -	  value_range res;
>> +	  Value_Range res (vr_type);
>>   
>> -	  if (ipa_vr_operation_and_type_effects (&res,
>> -						 &srcvr,
>> +	  if (ipa_vr_operation_and_type_effects (res,
>> +						 srcvr,
>>   						 operation, parm_type,
>>   						 vr_type))
>>   	    vr.intersect (res);
>>   	}
>>         else
>>   	{
>> -	  value_range op_res, res;
>> +	  Value_Range op_res (vr_type);
>> +	  Value_Range res (vr_type);
>>   	  tree op = ipa_get_jf_pass_through_operand (jfunc);
>> -	  value_range op_vr;
>> +	  Value_Range op_vr (vr_type);
>>   	  range_op_handler handler (operation, vr_type);
>>   
>>   	  ipa_range_set_and_normalize (op_vr, op);
>> @@ -1979,14 +1999,13 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs,
>>   	      || !handler.fold_range (op_res, vr_type, srcvr, op_vr))
>>   	    op_res.set_varying (vr_type);
>>   
>> -	  if (ipa_vr_operation_and_type_effects (&res,
>> -						 &op_res,
>> +	  if (ipa_vr_operation_and_type_effects (res,
>> +						 op_res,
>>   						 NOP_EXPR, parm_type,
>>   						 vr_type))
>>   	    vr.intersect (res);
>>   	}
>>       }
>> -  return vr;
>>   }
>>   
>>   /* Determine whether ITEM, jump function for an aggregate part, evaluates to a
>> @@ -2753,10 +2772,10 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>         if (src_lats->m_value_range.bottom_p ())
>>   	return dest_lat->set_to_bottom ();
>>   
>> -      value_range vr;
>> +      Value_Range vr (operand_type);
>>         if (TREE_CODE_CLASS (operation) == tcc_unary)
>> -	ipa_vr_operation_and_type_effects (&vr,
>> -					   &src_lats->m_value_range.m_vr,
>> +	ipa_vr_operation_and_type_effects (vr,
>> +					   src_lats->m_value_range.m_vr,
>>   					   operation, param_type,
>>   					   operand_type);
>>         /* A crude way to prevent unbounded number of value range updates
>> @@ -2765,8 +2784,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>         else if (!ipa_edge_within_scc (cs))
>>   	{
>>   	  tree op = ipa_get_jf_pass_through_operand (jfunc);
>> -	  value_range op_vr;
>> -	  value_range op_res,res;
>> +	  Value_Range op_vr (TREE_TYPE (op));
>> +	  Value_Range op_res (operand_type);
>>   	  range_op_handler handler (operation, operand_type);
>>   
>>   	  ipa_range_set_and_normalize (op_vr, op);
>> @@ -2777,8 +2796,8 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>   				      src_lats->m_value_range.m_vr, op_vr))
>>   	    op_res.set_varying (operand_type);
>>   
>> -	  ipa_vr_operation_and_type_effects (&vr,
>> -					     &op_res,
>> +	  ipa_vr_operation_and_type_effects (vr,
>> +					     op_res,
>>   					     NOP_EXPR, param_type,
>>   					     operand_type);
>>   	}
>> @@ -2786,14 +2805,14 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>   	{
>>   	  if (jfunc->m_vr)
>>   	    {
>> -	      value_range jvr;
>> -	      if (ipa_vr_operation_and_type_effects (&jvr, jfunc->m_vr,
>> +	      Value_Range jvr (param_type);
>> +	      if (ipa_vr_operation_and_type_effects (jvr, *jfunc->m_vr,
>>   						     NOP_EXPR,
>>   						     param_type,
>>   						     jfunc->m_vr->type ()))
>>   		vr.intersect (jvr);
>>   	    }
>> -	  return dest_lat->meet_with (&vr);
>> +	  return dest_lat->meet_with (vr);
>>   	}
>>       }
>>     else if (jfunc->type == IPA_JF_CONST)
>> @@ -2805,18 +2824,17 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>>   	  if (TREE_OVERFLOW_P (val))
>>   	    val = drop_tree_overflow (val);
>>   
>> -	  value_range tmpvr (TREE_TYPE (val),
>> -			     wi::to_wide (val), wi::to_wide (val));
>> -	  return dest_lat->meet_with (&tmpvr);
>> +	  Value_Range tmpvr (val, val);
>> +	  return dest_lat->meet_with (tmpvr);
>>   	}
>>       }
>>   
>> -  value_range vr;
>> +  Value_Range vr (param_type);
>>     if (jfunc->m_vr
>> -      && ipa_vr_operation_and_type_effects (&vr, jfunc->m_vr, NOP_EXPR,
>> +      && ipa_vr_operation_and_type_effects (vr, *jfunc->m_vr, NOP_EXPR,
>>   					    param_type,
>>   					    jfunc->m_vr->type ()))
>> -    return dest_lat->meet_with (&vr);
>> +    return dest_lat->meet_with (vr);
>>     else
>>       return dest_lat->set_to_bottom ();
>>   }
>> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
>> index b328bb8ce14..cf416202920 100644
>> --- a/gcc/ipa-fnsummary.cc
>> +++ b/gcc/ipa-fnsummary.cc
>> @@ -475,7 +475,7 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
>>   	  && !c->agg_contents
>>   	  && (!val || TREE_CODE (val) != INTEGER_CST))
>>   	{
>> -	  value_range vr = avals->m_known_value_ranges[c->operand_num];
>> +	  Value_Range vr (avals->m_known_value_ranges[c->operand_num]);
>>   	  if (!vr.undefined_p ()
>>   	      && !vr.varying_p ()
>>   	      && (TYPE_SIZE (c->type) == TYPE_SIZE (vr.type ())))
>> @@ -630,8 +630,8 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>   		|| ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>>   	      {
>>   		/* Determine if we know constant value of the parameter.  */
>> -		tree cst = ipa_value_from_jfunc (caller_parms_info, jf,
>> -						 ipa_get_type (callee_pi, i));
>> +		tree type = ipa_get_type (callee_pi, i);
>> +		tree cst = ipa_value_from_jfunc (caller_parms_info, jf, type);
>>   
>>   		if (!cst && e->call_stmt
>>   		    && i < (int)gimple_call_num_args (e->call_stmt))
>> @@ -659,10 +659,9 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>   		    && vrp_will_run_p (caller)
>>   		    && ipa_is_param_used_by_ipa_predicates (callee_pi, i))
>>   		  {
>> -		    value_range vr
>> -		       = ipa_value_range_from_jfunc (caller_parms_info, e, jf,
>> -						     ipa_get_type (callee_pi,
>> -								   i));
>> +		    Value_Range vr (type);
>> +
>> +		    ipa_value_range_from_jfunc (vr, caller_parms_info, e, jf, type);
>>   		    if (!vr.undefined_p () && !vr.varying_p ())
>>   		      {
>>   			if (!avals->m_known_value_ranges.length ())
>> @@ -670,7 +669,7 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
>>   			    avals->m_known_value_ranges.safe_grow (count, true);
>>   			    for (int i = 0; i < count; ++i)
>>   			      new (&avals->m_known_value_ranges[i])
>> -				value_range ();
>> +				Value_Range ();
>>   			  }
>>   			avals->m_known_value_ranges[i] = vr;
>>   		      }
>> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
>> index ab6de9f10da..bbfe0f8aa45 100644
>> --- a/gcc/ipa-prop.cc
>> +++ b/gcc/ipa-prop.cc
>> @@ -198,8 +198,9 @@ ipa_vr::equal_p (const vrange &r) const
>>   }
>>   
>>   void
>> -ipa_vr::get_vrange (vrange &r) const
>> +ipa_vr::get_vrange (Value_Range &r) const
>>   {
>> +  r.set_type (m_type);
>>     m_storage->get_vrange (r, m_type);
>>   }
>>   
>> @@ -5963,7 +5964,7 @@ ipcp_update_vr (struct cgraph_node *node)
>>   
>>         if (vr[i].known_p ())
>>   	{
>> -	  value_range tmp;
>> +	  Value_Range tmp;
>>   	  vr[i].get_vrange (tmp);
>>   
>>   	  if (!tmp.undefined_p () && !tmp.varying_p ())
>> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
>> index f306f8a377e..3a591a8f44d 100644
>> --- a/gcc/ipa-prop.h
>> +++ b/gcc/ipa-prop.h
>> @@ -314,7 +314,7 @@ public:
>>     void set_unknown ();
>>     bool known_p () const { return m_storage != NULL; }
>>     tree type () const { return m_type; }
>> -  void get_vrange (vrange &) const;
>> +  void get_vrange (Value_Range &) const;
>>     bool equal_p (const vrange &) const;
>>     const vrange_storage *storage () const { return m_storage; }
>>     void streamer_read (lto_input_block *, data_in *);
>> @@ -530,7 +530,7 @@ public:
>>     auto_vec<ipa_argagg_value, 32> m_known_aggs;
>>   
>>     /* Vector describing known value ranges of arguments.  */
>> -  auto_vec<value_range, 32> m_known_value_ranges;
>> +  auto_vec<Value_Range, 32> m_known_value_ranges;
>>   };
>>   
>>   inline
>> @@ -582,7 +582,7 @@ public:
>>     vec<ipa_argagg_value> m_known_aggs = vNULL;
>>   
>>     /* Vector describing known value ranges of arguments.  */
>> -  vec<value_range> m_known_value_ranges = vNULL;
>> +  vec<Value_Range> m_known_value_ranges = vNULL;
>>   };
>>   
>>   inline
>> @@ -1194,8 +1194,8 @@ ipa_polymorphic_call_context ipa_context_from_jfunc (ipa_node_params *,
>>   						     cgraph_edge *,
>>   						     int,
>>   						     ipa_jump_func *);
>> -value_range ipa_value_range_from_jfunc (ipa_node_params *, cgraph_edge *,
>> -					ipa_jump_func *, tree);
>> +void ipa_value_range_from_jfunc (vrange &, ipa_node_params *, cgraph_edge *,
>> +				 ipa_jump_func *, tree);
>>   void ipa_push_agg_values_from_jfunc (ipa_node_params *info, cgraph_node *node,
>>   				     ipa_agg_jump_function *agg_jfunc,
>>   				     unsigned dst_index,
>> @@ -1218,17 +1218,12 @@ void ipa_cp_cc_finalize (void);
>>      non-zero.  */
>>   
>>   inline void
>> -ipa_range_set_and_normalize (irange &r, tree val)
>> +ipa_range_set_and_normalize (vrange &r, tree val)
>>   {
>> -  if (TREE_CODE (val) == INTEGER_CST)
>> -    {
>> -      wide_int w = wi::to_wide (val);
>> -      r.set (TREE_TYPE (val), w, w);
>> -    }
>> -  else if (TREE_CODE (val) == ADDR_EXPR)
>> +  if (TREE_CODE (val) == ADDR_EXPR)
>>       r.set_nonzero (TREE_TYPE (val));
>>     else
>> -    r.set_varying (TREE_TYPE (val));
>> +    r.set (val, val);
>>   }
>>   
>>   #endif /* IPA_PROP_H */
>> -- 
>> 2.40.1
>