On Fri, May 9, 2025 at 1:34 PM Andrew Pinski <pins...@gmail.com> wrote:
>
> On Fri, May 9, 2025 at 1:21 AM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Fri, May 9, 2025 at 4:51 AM Andrew Pinski <quic_apin...@quicinc.com>
> > wrote:
> > >
> > > These patterns are not needed any more. There were already
> > > 2 patterns which did `(ne bool_var 0)` into `bool_var` and
> > > `(eq bool_var 1)` into `bool_var`. Just they were after the
> > > pattern that did `(cmp (cond @0 @1 @2) @3)` simplification but
> > > that pattern is now after the ones.
> > > Also these patterns will cause in some cases a new statement to
> > > be created for the comparison. In the case of floating point comparison
> > > wiht non-call exceptions (and trapping math), can cause a new statement
> > > every time fold_stmt is called.
> >
> > Hmm, but do we still fold
> >
> > _1 = _2 < 1;
> > if (_1 != 0)
> >
> > to
> >
> > if (_2 < 1)
> >
> > or does that now again rely on forwprops explicit forwarding into
> > gcond? I wanted
> > to get rid of the latter eventually.
>
> Oh. Yes this does rely on forwprop explicitly now.
>
> >
> > I agree that the trapping math thing is bad - I wonder if we can catch that
> > more
> > intelligently (not sure how without following SSA use-def of gconds on bools
> > and see whether they can trap and then not simplifying)
>
> I think I know the way to fix the trapping issue without fully
> removing this. I am going to give it a go later today.
> Since trapping only depends on the code and the type it should be easy
> to add an extra condition here and the latter patterns catch the
> trapping case of removing `bool!=0` already.
Note it's really depending on context.
_1 = _2 < 1.;
_3 = _1 != 0;
would be OK to fold to
_3 = _2 < 1;
but not with the _1 != 0 in the gcond. That's because gconds can't
throw (and I think
rightfully so). In principle we should go full steam ahead to have
single-operand
gconds, just the boolean value. Like we now do for COND_EXPRs. But
this unfortunately
has very large fallout :/
Thus the "workaround" for non-call-EH. I believe any mitigation should be in
the match-and-simplify plumbing that handles the gcond - which we already do,
but the side-effect is the ping-pong you are observing. Maybe we can do
better in replace_stmt_with_simplification where we should hit(?)
else if (!inplace)
{
tree res = maybe_push_res_to_seq (res_op, seq);
if (!res)
return false;
gimple_cond_set_condition (cond_stmt, NE_EXPR, res,
build_zero_cst (TREE_TYPE (res)));
and detect when the cond_stmt is SSA != 0 (or the reverse canonical form)
and refuse to simplify if the simplification in 'res_op' is the same as the
current definition of SSA?
>
> Thanks,
> Andrew Pinski
>
> >
> > > gcc.dg/tree-ssa/vrp24.c needed to be adjusted to before
> > > r13-322-g7f04b0d786e13f.
> > > gcc.dg/analyzer/null-deref-pr102671-2.c needs an increased
> > > analyzer-max-svalue-depth
> > > not to get an extra warning.
> > >
> > > gcc/ChangeLog:
> > >
> > > * match.pd (`(ne (cmp) 0)`, `(eq (cmp) 1)`): Remove.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * gcc.dg/tree-ssa/vrp24.c: Adjust.
> > > * gcc.dg/analyzer/null-deref-pr102671-2.c: Increase
> > > analyzer-max-svalue-depth.
> > >
> > > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > > ---
> > > gcc/match.pd | 8 --------
> > > gcc/testsuite/gcc.dg/analyzer/null-deref-pr102671-2.c | 2 +-
> > > gcc/testsuite/gcc.dg/tree-ssa/vrp24.c | 2 +-
> > > 3 files changed, 2 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/gcc/match.pd b/gcc/match.pd
> > > index ab496d923cc..418efc4230a 100644
> > > --- a/gcc/match.pd
> > > +++ b/gcc/match.pd
> > > @@ -6898,14 +6898,6 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > > (if (ic == ncmp)
> > > (ncmp @0 @1)))))
> > > /* The following bits are handled by
> > > fold_binary_op_with_conditional_arg. */
> > > - (simplify
> > > - (ne (cmp@2 @0 @1) integer_zerop)
> > > - (if (types_match (type, TREE_TYPE (@2)))
> > > - (cmp @0 @1)))
> > > - (simplify
> > > - (eq (cmp@2 @0 @1) integer_truep)
> > > - (if (types_match (type, TREE_TYPE (@2)))
> > > - (cmp @0 @1)))
> > > (simplify
> > > (ne (cmp@2 @0 @1) integer_truep)
> > > (if (types_match (type, TREE_TYPE (@2)))
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/null-deref-pr102671-2.c
> > > b/gcc/testsuite/gcc.dg/analyzer/null-deref-pr102671-2.c
> > > index 298e4839b98..bc141d5c028 100644
> > > --- a/gcc/testsuite/gcc.dg/analyzer/null-deref-pr102671-2.c
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/null-deref-pr102671-2.c
> > > @@ -1,5 +1,5 @@
> > > /* { dg-require-effective-target ptr_eq_long } */
> > > -/* { dg-additional-options "-O2 -Wno-shift-count-overflow" } */
> > > +/* { dg-additional-options "-O2 -Wno-shift-count-overflow
> > > --param=analyzer-max-svalue-depth=19" } */
> > >
> > > struct lisp;
> > > union vectorlike_header { long size; };
> > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp24.c
> > > b/gcc/testsuite/gcc.dg/tree-ssa/vrp24.c
> > > index c28ca473fc6..f237b7741ec 100644
> > > --- a/gcc/testsuite/gcc.dg/tree-ssa/vrp24.c
> > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp24.c
> > > @@ -89,5 +89,5 @@ L7:
> > > boolean operation. */
> > >
> > > /* { dg-final { scan-tree-dump-times "Simplified relational" 2 "evrp" }
> > > } */
> > > -/* { dg-final { scan-tree-dump-times "if " 3 "optimized" } } */
> > > +/* { dg-final { scan-tree-dump-times "if " 4 "optimized" } } */
> > >
> > > --
> > > 2.43.0
> > >
