| Message ID | 20230426083328.313566-4-aldyh@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [COMMITTED] Remove deprecated range_fold_{unary, binary}_expr uses from ipa-*. | expand |
Hello, On Wed, Apr 26 2023, Aldy Hernandez via Gcc-patches wrote: > gcc/ChangeLog: > > * ipa-cp.cc (ipa_vr_operation_and_type_effects): Convert to ranger API. > (ipa_value_range_from_jfunc): Same. > (propagate_vr_across_jump_function): Same. > * ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same. > * ipa-prop.cc (ipa_compute_jump_functions_for_edge): Same. > * vr-values.cc (bounds_of_var_in_loop): Same. thanks for taking care of the value range uses in IPA. > --- > gcc/ipa-cp.cc | 28 +++++++++++++++++++++------ > gcc/ipa-fnsummary.cc | 45 ++++++++++++++++++++++++++++---------------- > gcc/ipa-prop.cc | 5 ++--- > gcc/vr-values.cc | 6 ++++-- > 4 files changed, 57 insertions(+), 27 deletions(-) > > diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc > index 65c49558b58..6788883c40b 100644 > --- a/gcc/ipa-cp.cc > +++ b/gcc/ipa-cp.cc > @@ -128,6 +128,7 @@ along with GCC; see the file COPYING3. If not see > #include "attribs.h" > #include "dbgcnt.h" > #include "symtab-clones.h" > +#include "gimple-range.h" > > template <typename valtype> class ipcp_value; > > @@ -1900,10 +1901,15 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr, > enum tree_code operation, > tree dst_type, tree src_type) > { > - range_fold_unary_expr (dst_vr, operation, dst_type, src_vr, src_type); > - if (dst_vr->varying_p () || dst_vr->undefined_p ()) > + if (!irange::supports_p (dst_type) || !irange::supports_p (src_type)) > return false; > - return true; > + > + range_op_handler handler (operation, dst_type); Would it be possible to document the range_op_handler class somewhat? > + return (handler > + && handler.fold_range (*dst_vr, dst_type, > + *src_vr, value_range (dst_type)) > + && !dst_vr->varying_p () > + && !dst_vr->undefined_p ()); It looks important but the class is not documented at all. Although the use of fold_range is probably hopefully mostly clear from its uses in this patch, the meaning of the return value of this method and what other methods do is less obvious. For example, I am curious why (not in this patch, but in the code as it is now in the repo), uses of fold_range seem to be always preceeded with a check for supports_type_p, even though the type is then also fed into fold_range itself. Does the return value of fold_range mean something slightly different from "could not deduce anything?" Thanks! Martin
On 5/5/23 17:10, Martin Jambor wrote: > Hello, > > On Wed, Apr 26 2023, Aldy Hernandez via Gcc-patches wrote: >> gcc/ChangeLog: >> >> * ipa-cp.cc (ipa_vr_operation_and_type_effects): Convert to ranger API. >> (ipa_value_range_from_jfunc): Same. >> (propagate_vr_across_jump_function): Same. >> * ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same. >> * ipa-prop.cc (ipa_compute_jump_functions_for_edge): Same. >> * vr-values.cc (bounds_of_var_in_loop): Same. > > thanks for taking care of the value range uses in IPA. > >> --- >> gcc/ipa-cp.cc | 28 +++++++++++++++++++++------ >> gcc/ipa-fnsummary.cc | 45 ++++++++++++++++++++++++++++---------------- >> gcc/ipa-prop.cc | 5 ++--- >> gcc/vr-values.cc | 6 ++++-- >> 4 files changed, 57 insertions(+), 27 deletions(-) >> >> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc >> index 65c49558b58..6788883c40b 100644 >> --- a/gcc/ipa-cp.cc >> +++ b/gcc/ipa-cp.cc >> @@ -128,6 +128,7 @@ along with GCC; see the file COPYING3. If not see >> #include "attribs.h" >> #include "dbgcnt.h" >> #include "symtab-clones.h" >> +#include "gimple-range.h" >> >> template <typename valtype> class ipcp_value; >> >> @@ -1900,10 +1901,15 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr, >> enum tree_code operation, >> tree dst_type, tree src_type) >> { >> - range_fold_unary_expr (dst_vr, operation, dst_type, src_vr, src_type); >> - if (dst_vr->varying_p () || dst_vr->undefined_p ()) >> + if (!irange::supports_p (dst_type) || !irange::supports_p (src_type)) >> return false; >> - return true; >> + >> + range_op_handler handler (operation, dst_type); > > Would it be possible to document the range_op_handler class somewhat? Sorry for the late response, but you're totally right. We're in dire need of documentation here. I had planned to work on comments and actual documentation much later this cycle, but I may need to bump that up in priority. > >> + return (handler >> + && handler.fold_range (*dst_vr, dst_type, >> + *src_vr, value_range (dst_type)) >> + && !dst_vr->varying_p () >> + && !dst_vr->undefined_p ()); > > It looks important but the class is not documented at all. Although the > use of fold_range is probably hopefully mostly clear from its uses in > this patch, the meaning of the return value of this method and what > other methods do is less obvious. > > For example, I am curious why (not in this patch, but in the code as it > is now in the repo), uses of fold_range seem to be always preceeded with > a check for supports_type_p, even though the type is then also fed into > fold_range itself. Does the return value of fold_range mean something > slightly different from "could not deduce anything?" Returning false from fold_range() is a shortcut for I don't know anything, which will be treated as VARYING upstream. The other methods also need documentation. The most important ones are documented in range-op.h: // This class is implemented for each kind of operator supported by // the range generator. It serves various purposes. particularly op1_range, and op2_range which can be confusing. But yes, we need to revisit this, as those comments are pretty out of date. Aldy
On 5/5/23 17:10, Martin Jambor wrote: > Hello, > > On Wed, Apr 26 2023, Aldy Hernandez via Gcc-patches wrote: >> gcc/ChangeLog: >> >> * ipa-cp.cc (ipa_vr_operation_and_type_effects): Convert to ranger API. >> (ipa_value_range_from_jfunc): Same. >> (propagate_vr_across_jump_function): Same. >> * ipa-fnsummary.cc (evaluate_conditions_for_known_args): Same. >> * ipa-prop.cc (ipa_compute_jump_functions_for_edge): Same. >> * vr-values.cc (bounds_of_var_in_loop): Same. > > thanks for taking care of the value range uses in IPA. > >> --- >> gcc/ipa-cp.cc | 28 +++++++++++++++++++++------ >> gcc/ipa-fnsummary.cc | 45 ++++++++++++++++++++++++++++---------------- >> gcc/ipa-prop.cc | 5 ++--- >> gcc/vr-values.cc | 6 ++++-- >> 4 files changed, 57 insertions(+), 27 deletions(-) >> >> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc >> index 65c49558b58..6788883c40b 100644 >> --- a/gcc/ipa-cp.cc >> +++ b/gcc/ipa-cp.cc >> @@ -128,6 +128,7 @@ along with GCC; see the file COPYING3. If not see >> #include "attribs.h" >> #include "dbgcnt.h" >> #include "symtab-clones.h" >> +#include "gimple-range.h" >> >> template <typename valtype> class ipcp_value; >> >> @@ -1900,10 +1901,15 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr, >> enum tree_code operation, >> tree dst_type, tree src_type) >> { >> - range_fold_unary_expr (dst_vr, operation, dst_type, src_vr, src_type); >> - if (dst_vr->varying_p () || dst_vr->undefined_p ()) >> + if (!irange::supports_p (dst_type) || !irange::supports_p (src_type)) >> return false; >> - return true; >> + >> + range_op_handler handler (operation, dst_type); > > Would it be possible to document the range_op_handler class somewhat? > >> + return (handler >> + && handler.fold_range (*dst_vr, dst_type, >> + *src_vr, value_range (dst_type)) >> + && !dst_vr->varying_p () >> + && !dst_vr->undefined_p ()); > > It looks important but the class is not documented at all. Although the > use of fold_range is probably hopefully mostly clear from its uses in > this patch, the meaning of the return value of this method and what > other methods do is less obvious. > > For example, I am curious why (not in this patch, but in the code as it > is now in the repo), uses of fold_range seem to be always preceeded with > a check for supports_type_p, even though the type is then also fed into > fold_range itself. Does the return value of fold_range mean something > slightly different from "could not deduce anything?" Oh, I see what you mean. Take for instance this bit in ipa-cp: if (!irange::supports_p (dst_type) || !irange::supports_p (src_type)) 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 ()); range_op_handler::fold_range() takes a type agnostic vrange (from which irange inherits). If you pass it an irange, but the type is say a float, you'll get an ICE downstream. Ranger itself is type agnostic and takes a vrange almost everywhere. It's up to the user to make sure the the range type and the type of the operation matches. Eventually we should convert all those value_range arguments in IPA to vrange and have it work in a type agnostic manner. I have patches for this, but I still have to flush out all this preliminary stuff :). Aldy
diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc index 65c49558b58..6788883c40b 100644 --- a/gcc/ipa-cp.cc +++ b/gcc/ipa-cp.cc @@ -128,6 +128,7 @@ along with GCC; see the file COPYING3. If not see #include "attribs.h" #include "dbgcnt.h" #include "symtab-clones.h" +#include "gimple-range.h" template <typename valtype> class ipcp_value; @@ -1900,10 +1901,15 @@ ipa_vr_operation_and_type_effects (value_range *dst_vr, enum tree_code operation, tree dst_type, tree src_type) { - range_fold_unary_expr (dst_vr, operation, dst_type, src_vr, src_type); - if (dst_vr->varying_p () || dst_vr->undefined_p ()) + if (!irange::supports_p (dst_type) || !irange::supports_p (src_type)) return false; - return true; + + 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 ()); } /* Determine value_range of JFUNC given that INFO describes the caller node or @@ -1958,8 +1964,13 @@ ipa_value_range_from_jfunc (ipa_node_params *info, cgraph_edge *cs, value_range op_res, res; tree op = ipa_get_jf_pass_through_operand (jfunc); value_range op_vr (op, op); + range_op_handler handler (operation, vr_type); + + if (!handler + || !op_res.supports_type_p (vr_type) + || !handler.fold_range (op_res, vr_type, srcvr, op_vr)) + op_res.set_varying (vr_type); - range_fold_binary_expr (&op_res, operation, vr_type, &srcvr, &op_vr); if (ipa_vr_operation_and_type_effects (&res, &op_res, NOP_EXPR, parm_type, @@ -2748,9 +2759,14 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc, tree op = ipa_get_jf_pass_through_operand (jfunc); value_range op_vr (op, op); value_range op_res,res; + range_op_handler handler (operation, operand_type); + + if (!handler + || !op_res.supports_type_p (operand_type) + || !handler.fold_range (op_res, operand_type, + src_lats->m_value_range.m_vr, op_vr)) + op_res.set_varying (operand_type); - range_fold_binary_expr (&op_res, operation, operand_type, - &src_lats->m_value_range.m_vr, &op_vr); ipa_vr_operation_and_type_effects (&vr, &op_res, NOP_EXPR, param_type, diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc index d4b2a073240..03142960e60 100644 --- a/gcc/ipa-fnsummary.cc +++ b/gcc/ipa-fnsummary.cc @@ -481,13 +481,7 @@ evaluate_conditions_for_known_args (struct cgraph_node *node, && (TYPE_SIZE (c->type) == TYPE_SIZE (vr.type ()))) { if (!useless_type_conversion_p (c->type, vr.type ())) - { - value_range res; - range_fold_unary_expr (&res, NOP_EXPR, - c->type, &vr, vr.type ()); - vr = res; - } - tree type = c->type; + range_cast (vr, c->type); for (j = 0; vec_safe_iterate (c->param_ops, j, &op); j++) { @@ -496,26 +490,45 @@ evaluate_conditions_for_known_args (struct cgraph_node *node, value_range res; if (!op->val[0]) - range_fold_unary_expr (&res, op->code, op->type, &vr, type); + { + range_op_handler handler (op->code, op->type); + if (!handler + || !res.supports_type_p (op->type) + || !handler.fold_range (res, op->type, vr, + value_range (op->type))) + res.set_varying (op->type); + } else if (!op->val[1]) { value_range op0 (op->val[0], op->val[0]); - range_fold_binary_expr (&res, op->code, op->type, - op->index ? &op0 : &vr, - op->index ? &vr : &op0); + range_op_handler handler (op->code, op->type); + + if (!handler + || !res.supports_type_p (op->type) + || !handler.fold_range (res, op->type, + op->index ? op0 : vr, + op->index ? vr : op0)) + res.set_varying (op->type); } else res.set_varying (op->type); - type = op->type; vr = res; } if (!vr.varying_p () && !vr.undefined_p ()) { value_range res; - value_range val_vr (c->val, c->val); - range_fold_binary_expr (&res, c->code, boolean_type_node, - &vr, - &val_vr); + value_range val_vr; + if (TREE_CODE (c->val) == INTEGER_CST) + val_vr.set (c->val, c->val); + else + val_vr.set_varying (TREE_TYPE (c->val)); + range_op_handler handler (c->code, boolean_type_node); + + if (!handler + || !res.supports_type_p (boolean_type_node) + || !handler.fold_range (res, boolean_type_node, vr, val_vr)) + res.set_varying (boolean_type_node); + if (res.zero_p ()) continue; } diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc index a55cc72b0fd..f706f099f7a 100644 --- a/gcc/ipa-prop.cc +++ b/gcc/ipa-prop.cc @@ -2323,9 +2323,8 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi, && get_range_query (cfun)->range_of_expr (vr, arg) && !vr.undefined_p ()) { - value_range resvr; - range_fold_unary_expr (&resvr, NOP_EXPR, param_type, - &vr, TREE_TYPE (arg)); + value_range resvr = vr; + range_cast (resvr, param_type); if (!resvr.undefined_p () && !resvr.varying_p ()) ipa_set_jfunc_vr (jfunc, &resvr); else diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc index 41e12daf7f8..ea4fbd7af67 100644 --- a/gcc/vr-values.cc +++ b/gcc/vr-values.cc @@ -579,8 +579,10 @@ bounds_of_var_in_loop (tree *min, tree *max, range_query *query, vr0.set_varying (TREE_TYPE (init)); tree tem = wide_int_to_tree (TREE_TYPE (init), wtmp); vr1.set (tem, tem); - range_fold_binary_expr (&maxvr, PLUS_EXPR, - TREE_TYPE (init), &vr0, &vr1); + + range_op_handler handler (PLUS_EXPR, TREE_TYPE (init)); + if (!handler.fold_range (maxvr, TREE_TYPE (init), vr0, vr1)) + maxvr.set_varying (TREE_TYPE (init)); /* Likewise if the addition did. */ if (maxvr.kind () == VR_RANGE)