On Fri, May 10, 2024 at 11:24 AM Aldy Hernandez <al...@redhat.com> wrote: > > There are various calls into fold_range() that have the wrong type > associated with the range temporary used to hold the result. This > used to work, because we could store either integers or pointers in a > Value_Range, but is no longer the case with prange's. Now you must > explicitly state which type of range the temporary will hold before > storing into it. You can change this at a later time with set_type(), > but you must always have a type before using the temporary, and it > must match what fold_range() returns. > > This patch adjusts the IPA code to restore the previous functionality, > so I can re-enable the prange code, but I do question whether the > previous code was correct. I have added appropriate comments to help > the maintainers, but someone with more knowledge should revamp this > going forward. > > The basic problem is that pointer comparisons return a boolean, but > the IPA code is initializing the resulting range as a pointer. This > wasn't a problem, because fold_range() would previously happily force > the range into an integer one, and everything would work. But now we > must initialize the range to an integer before calling into > fold_range. The thing is, that the failing case sets the result back > into a pointer, which is just weird but existing behavior. I have > documented this in the code. > > if (!handler > || !op_res.supports_type_p (vr_type) > || !handler.fold_range (op_res, vr_type, srcvr, op_vr)) > /* For comparison operators, the type here may be > different than the range type used in fold_range above. > For example, vr_type may be a pointer, whereas the type > returned by fold_range will always be a boolean. > > This shouldn't cause any problems, as the set_varying > below will happily change the type of the range in > op_res, and then the cast operation in > ipa_vr_operation_and_type_effects will ultimately leave > things in the desired type, but it is confusing. > > Perhaps the original intent was to use the type of > op_res here? */ > op_res.set_varying (vr_type); > > BTW, this is not to say that the original gimple IR was wrong, but that > IPA is setting the range type of the result of fold_range() to the type of > the operands, which does not necessarily match in the case of a > comparison. > > I am just restoring previous behavior here, but I do question whether it > was right to begin with. > > Testing currently in progress on x86-64 and ppc64le with prange enabled. > > OK pending tests?
I think this "intermediate" patch is unnecessary and instead the code should be fixed correctly, avoiding missed-optimization regressions. Richard. > gcc/ChangeLog: > > PR tree-optimization/114985 > * ipa-cp.cc (ipa_value_range_from_jfunc): Adjust type of op_res. > (propagate_vr_across_jump_function): Same. > * ipa-fnsummary.cc (evaluate_conditions_for_known_args): Adjust > type for res. > * ipa-prop.h (ipa_type_for_fold_range): New. > --- > gcc/ipa-cp.cc | 18 ++++++++++++++++-- > gcc/ipa-fnsummary.cc | 6 +++++- > gcc/ipa-prop.h | 13 +++++++++++++ > 3 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc > index 5781f50c854..3c395632364 100644 > --- a/gcc/ipa-cp.cc > +++ b/gcc/ipa-cp.cc > @@ -1730,7 +1730,7 @@ ipa_value_range_from_jfunc (vrange &vr, > } > else > { > - Value_Range op_res (vr_type); > + Value_Range op_res (ipa_type_for_fold_range (operation, vr_type)); > Value_Range res (vr_type); > tree op = ipa_get_jf_pass_through_operand (jfunc); > Value_Range op_vr (TREE_TYPE (op)); > @@ -1741,6 +1741,19 @@ ipa_value_range_from_jfunc (vrange &vr, > if (!handler > || !op_res.supports_type_p (vr_type) > || !handler.fold_range (op_res, vr_type, srcvr, op_vr)) > + /* For comparison operators, the type here may be > + different than the range type used in fold_range above. > + For example, vr_type may be a pointer, whereas the type > + returned by fold_range will always be a boolean. > + > + This shouldn't cause any problems, as the set_varying > + below will happily change the type of the range in > + op_res, and then the cast operation in > + ipa_vr_operation_and_type_effects will ultimately leave > + things in the desired type, but it is confusing. > + > + Perhaps the original intent was to use the type of > + op_res here? */ > op_res.set_varying (vr_type); > > if (ipa_vr_operation_and_type_effects (res, > @@ -2540,7 +2553,7 @@ 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 (TREE_TYPE (op)); > - Value_Range op_res (param_type); > + Value_Range op_res (ipa_type_for_fold_range (operation, > param_type)); > range_op_handler handler (operation); > > ipa_range_set_and_normalize (op_vr, op); > @@ -2549,6 +2562,7 @@ propagate_vr_across_jump_function (cgraph_edge *cs, > ipa_jump_func *jfunc, > || !ipa_supports_p (operand_type) > || !handler.fold_range (op_res, operand_type, > src_lats->m_value_range.m_vr, op_vr)) > + /* See note in ipa_value_range_from_jfunc. */ > op_res.set_varying (param_type); > > ipa_vr_operation_and_type_effects (vr, > diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc > index 07a853f78e3..4deba2394f5 100644 > --- a/gcc/ipa-fnsummary.cc > +++ b/gcc/ipa-fnsummary.cc > @@ -502,7 +502,8 @@ evaluate_conditions_for_known_args (struct cgraph_node > *node, > if (vr.varying_p () || vr.undefined_p ()) > break; > > - Value_Range res (op->type); > + Value_Range res (ipa_type_for_fold_range (op->code, > + op->type)); > if (!op->val[0]) > { > Value_Range varying (op->type); > @@ -511,6 +512,7 @@ evaluate_conditions_for_known_args (struct cgraph_node > *node, > if (!handler > || !res.supports_type_p (op->type) > || !handler.fold_range (res, op->type, vr, varying)) > + /* See note in ipa_value_from_jfunc. */ > res.set_varying (op->type); > } > else if (!op->val[1]) > @@ -525,9 +527,11 @@ evaluate_conditions_for_known_args (struct cgraph_node > *node, > || !handler.fold_range (res, op->type, > op->index ? op0 : vr, > op->index ? vr : op0)) > + /* See note in ipa_value_from_jfunc. */ > res.set_varying (op->type); > } > else > + /* See note in ipa_value_from_jfunc. */ > res.set_varying (op->type); > vr = res; > } > diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h > index 93d1b87b1f7..8493dd19b92 100644 > --- a/gcc/ipa-prop.h > +++ b/gcc/ipa-prop.h > @@ -1277,6 +1277,19 @@ ipa_range_set_and_normalize (vrange &r, tree val) > r.set (val, val); > } > > +/* Return the resulting type for a fold_range() operation for OP and > + TYPE. */ > + > +inline tree > +ipa_type_for_fold_range (tree_code op, tree type) > +{ > + /* Comparisons return boolean regardless of their input operands. */ > + if (TREE_CODE_CLASS (op) == tcc_comparison) > + return boolean_type_node; > + > + return type; > +} > + > bool ipa_return_value_range (Value_Range &range, tree decl); > void ipa_record_return_value_range (Value_Range val); > bool ipa_jump_functions_equivalent_p (ipa_jump_func *jf1, ipa_jump_func > *jf2); > -- > 2.45.0 >