I would much prefer the IPA experts to fix the pass, but I'm afraid I don't understand the code enough to do so.
Could someone lend a hand here? Aldy On Fri, May 10, 2024 at 12:26 PM Richard Biener <richard.guent...@gmail.com> wrote: > > 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 > > >