On Fri, May 9, 2025 at 6:27 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> 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?
I submitted 2 different patches.
One which modifies the match.pd pattern:
https://gcc.gnu.org/pipermail/gcc-patches/2025-May/683142.html.
The patch to replace_stmt_with_simplification :
https://gcc.gnu.org/pipermail/gcc-patches/2025-May/683171.html
In fact I started with the replace_stmt_with_simplification but I was
not 100% sure it was the best idea so I didn't submit it.
I am ok with either one too.
Thanks,
Andrew Pinski
>
> >
> > 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
> > > >
