Adjust range type of calls into fold_range for IPA passes [PR114985]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Testing passed
|
Commit Message
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(-)
Comments
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
>
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
> >
>
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
> >
>
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
> > >
> >
@@ -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,
@@ -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;
}
@@ -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);