From: Aldy Hernandez <aldyh@redhat.com>
To: Martin Jambor <mjambor@suse.cz>
Cc: Andrew MacLeod <amacleod@redhat.com>,
GCC patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Convert ipcp_vr_lattice to type agnostic framework.
Date: Wed, 7 Jun 2023 12:18:04 +0200 [thread overview]
Message-ID: <7a17934c-4ac4-f776-0980-1333c46a4aff@redhat.com> (raw)
In-Reply-To: <ri6353jawy5.fsf@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 25278 bytes --]
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
[-- Attachment #2: 0001-Convert-ipcp_vr_lattice-to-type-agnostic-framework.patch --]
[-- Type: text/x-patch, Size: 17049 bytes --]
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 ();
}
else
{
- plats->m_value_range.init ();
+ plats->m_value_range.init (type);
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
next prev parent reply other threads:[~2023-06-07 10:18 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-17 14:30 Aldy Hernandez
2023-05-17 14:40 ` Aldy Hernandez
2023-05-22 18:43 ` Aldy Hernandez
2023-05-26 16:17 ` Martin Jambor
2023-06-06 12:43 ` Aldy Hernandez
2023-06-07 10:18 ` Aldy Hernandez [this message]
2023-06-10 8:49 ` Martin Jambor
2023-06-10 20:24 ` Aldy Hernandez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7a17934c-4ac4-f776-0980-1333c46a4aff@redhat.com \
--to=aldyh@redhat.com \
--cc=amacleod@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=mjambor@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).