* [PATCH] Adjust range type of calls into fold_range for IPA passes [PR114985] @ 2024-05-10 9:23 Aldy Hernandez 2024-05-10 10:21 ` Richard Biener 0 siblings, 1 reply; 5+ messages in thread From: Aldy Hernandez @ 2024-05-10 9:23 UTC (permalink / raw) To: GCC patches; +Cc: Martin Jambor, Andrew MacLeod, Aldy Hernandez 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? 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Adjust range type of calls into fold_range for IPA passes [PR114985] 2024-05-10 9:23 [PATCH] Adjust range type of calls into fold_range for IPA passes [PR114985] Aldy Hernandez @ 2024-05-10 10:21 ` Richard Biener 2024-05-10 10:29 ` Aldy Hernandez 2024-05-11 9:43 ` Aldy Hernandez 0 siblings, 2 replies; 5+ messages in thread From: Richard Biener @ 2024-05-10 10:21 UTC (permalink / raw) To: Aldy Hernandez; +Cc: GCC patches, Martin Jambor, Andrew MacLeod On Fri, May 10, 2024 at 11:24 AM Aldy Hernandez <aldyh@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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Adjust range type of calls into fold_range for IPA passes [PR114985] 2024-05-10 10:21 ` Richard Biener @ 2024-05-10 10:29 ` Aldy Hernandez 2024-05-11 9:43 ` Aldy Hernandez 1 sibling, 0 replies; 5+ messages in thread From: Aldy Hernandez @ 2024-05-10 10:29 UTC (permalink / raw) To: Richard Biener; +Cc: GCC patches, Martin Jambor, Andrew MacLeod 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.guenther@gmail.com> wrote: > > On Fri, May 10, 2024 at 11:24 AM Aldy Hernandez <aldyh@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 > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Adjust range type of calls into fold_range for IPA passes [PR114985] 2024-05-10 10:21 ` Richard Biener 2024-05-10 10:29 ` Aldy Hernandez @ 2024-05-11 9:43 ` Aldy Hernandez 2024-05-15 12:06 ` Aldy Hernandez 1 sibling, 1 reply; 5+ messages in thread From: Aldy Hernandez @ 2024-05-11 9:43 UTC (permalink / raw) To: Richard Biener; +Cc: GCC patches, Martin Jambor, Andrew MacLeod I have pushed a few cleanups to make it easier to move forward without disturbing passes which are affected by IPA's mixing up the range types. As I explained in my previous patch, this restores the default behavior of silently returning VARYING when a range operator is unsupported in either a particular operator, or in the dispatch code. I would like to re-enable prange support, as IPA was already broken before the prange work, and the debugging trap can be turned off to analyze (#define TRAP_ON_UNHANDLED_POINTER_OPERATORS 1). I have re-tested the effects of re-enabling prange in current trunk: 1. x86-64/32 bootstraps with no regressions with and without the trap. 2. ppc64le bootstraps with no regressions, but fails with the trap. 3. aarch64 bootstraps, but fails with the trap (no space on compile farm to run tests) 4. sparc: bootstrap already broken, so I can't test. So, for the above 4 architectures things work as before, and we have a PR to track the IPA problem which doesn't seem to affect neither bootstrap nor tests. Does this sound reasonable? Aldy On Fri, May 10, 2024 at 12:26 PM Richard Biener <richard.guenther@gmail.com> wrote: > > On Fri, May 10, 2024 at 11:24 AM Aldy Hernandez <aldyh@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 > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Adjust range type of calls into fold_range for IPA passes [PR114985] 2024-05-11 9:43 ` Aldy Hernandez @ 2024-05-15 12:06 ` Aldy Hernandez 0 siblings, 0 replies; 5+ messages in thread From: Aldy Hernandez @ 2024-05-15 12:06 UTC (permalink / raw) To: Richard Biener; +Cc: GCC patches, Martin Jambor, Andrew MacLeod Any thoughts on this? If no one objects, I'll re-enable prange tomorrow. Aldy On Sat, May 11, 2024 at 11:43 AM Aldy Hernandez <aldyh@redhat.com> wrote: > > I have pushed a few cleanups to make it easier to move forward without > disturbing passes which are affected by IPA's mixing up the range > types. As I explained in my previous patch, this restores the default > behavior of silently returning VARYING when a range operator is > unsupported in either a particular operator, or in the dispatch code. > > I would like to re-enable prange support, as IPA was already broken > before the prange work, and the debugging trap can be turned off to > analyze (#define TRAP_ON_UNHANDLED_POINTER_OPERATORS 1). > > I have re-tested the effects of re-enabling prange in current trunk: > > 1. x86-64/32 bootstraps with no regressions with and without the trap. > 2. ppc64le bootstraps with no regressions, but fails with the trap. > 3. aarch64 bootstraps, but fails with the trap (no space on compile > farm to run tests) > 4. sparc: bootstrap already broken, so I can't test. > > So, for the above 4 architectures things work as before, and we have a > PR to track the IPA problem which doesn't seem to affect neither > bootstrap nor tests. > > Does this sound reasonable? > > Aldy > > On Fri, May 10, 2024 at 12:26 PM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > On Fri, May 10, 2024 at 11:24 AM Aldy Hernandez <aldyh@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 > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-05-15 12:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-05-10 9:23 [PATCH] Adjust range type of calls into fold_range for IPA passes [PR114985] Aldy Hernandez 2024-05-10 10:21 ` Richard Biener 2024-05-10 10:29 ` Aldy Hernandez 2024-05-11 9:43 ` Aldy Hernandez 2024-05-15 12:06 ` Aldy Hernandez
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).