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 <al...@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.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
> > >
> >
