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 <al...@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 <al...@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, Martinif (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 toDST_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 orthe 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
