[PATCH] Convert ipcp_vr_lattice to type agnostic framework.
Martin Jambor
mjambor@suse.cz
Fri May 26 16:17:54 GMT 2023
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
More information about the Gcc-patches
mailing list