On Tue, Apr 22, 2025 at 1:06 AM Andrew Pinski <[email protected]> wrote:
>
> On Mon, Apr 21, 2025 at 1:42 AM Richard Biener
> <[email protected]> wrote:
> >
> > On Thu, Apr 17, 2025 at 7:37 PM Andrew Pinski <[email protected]>
> > wrote:
> > >
> > > So unlike constants, address invariants are currently put first if
> > > used with a SSA NAME.
> > > It would be better if address invariants are consistent with constants
> > > and this patch changes that.
> > > gcc.dg/tree-ssa/pr118902-1.c is an example where this canonicalization
> > > can help. In it if `p` variable was a global variable, FRE (VN) would
> > > have figured
> > > it out that `a` could never be equal to `&p` inside the loop. But without
> > > the
> > > canonicalization we end up with `&p == a.0_1` which VN does try to handle
> > > for conditional
> > > VN.
> > >
> > > Bootstrapped and tested on x86_64.
> > >
> > > PR tree-optimization/118902
> > > gcc/ChangeLog:
> > >
> > > * fold-const.cc (tree_swap_operands_p): Place invariants in the
> > > first operand
> > > if not used with constants.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * gcc.dg/tree-ssa/pr118902-1.c: New test.
> > >
> > > Signed-off-by: Andrew Pinski <[email protected]>
> > > ---
> > > gcc/fold-const.cc | 6 ++++++
> > > gcc/testsuite/gcc.dg/tree-ssa/pr118902-1.c | 21 +++++++++++++++++++++
> > > 2 files changed, 27 insertions(+)
> > > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr118902-1.c
> > >
> > > diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> > > index 1275ef75315..c9471ea44b0 100644
> > > --- a/gcc/fold-const.cc
> > > +++ b/gcc/fold-const.cc
> > > @@ -7246,6 +7246,12 @@ tree_swap_operands_p (const_tree arg0, const_tree
> > > arg1)
> > > if (TREE_CONSTANT (arg0))
> > > return true;
> > >
> > > + /* Put invariant address in arg1. */
> > > + if (is_gimple_invariant_address (arg1))
> > > + return false;
> > > + if (is_gimple_invariant_address (arg0))
> > > + return true;
> >
> > We could make this cheaper by considering all ADDR_EXPRs here?
>
> Maybe. we should.
>
> >
> > I'll note that with this or the above
> >
> > /* Put SSA_NAMEs last. */
> > if (TREE_CODE (arg1) == SSA_NAME)
> > return false;
> > if (TREE_CODE (arg0) == SSA_NAME)
> > return true;
> >
> > is a bit redundant and contradicting, when we are in GIMPLE, at least.
> > I'd say on GIMPLE reversing the above to put SSA_NAMEs first would
> > solve the ADDR_EXPR issue as well.
> >
> > The idea of tree_swap_operands_p seems to be to put "simple" things
> > second, but on GIMPLE SSA_NAME is not simple. With GENERIC
> > this would put memory refs first, SSA_NAME second, which is reasonable.
>
>
> So looking into the history on this. I find this from you from 2007:
> https://inbox.sourceware.org/gcc-patches/[email protected]/
Likely fold_comparison and friends was just testing one variant,
possibly match.pd does better and correctly uses :c there.
Note in principle a SSA name is a sub-expression while a DECL is memory,
so while the change probably fixed some missed folding the reasoning seems
wrong.
> I do wonder if we need a better way of describing the difference
> between gimple and generic too.
It might be tempting to split the predicate into a GIMPLE and GENERIC
variant ... but I'm not sure that for example gimplification re-canonicalizes
upon GIMPLE building.
> >
> > I'd say since an ADDR_EXPR is always a "value" (not memory), putting it
> > last makes sense in general, whether invariant or not. Can you test that?
> > The issue with is_gimple_invariant_address is that it walks all handled
> > components.
>
> Yes I will try to make that change to see if that fixes the issue; I
> think it does. Note `&a[var]` is not simple but will be treated as
> such with having ADDR_EXPR here; the walk figures out if it is simple
> or not though.
Yeah, that mostly makes a difference on GENERIC though since on GIMPLE
all ADDR_EXPR operands are simple, &a[var] would be a separate SSA def.
The difference might be for &decl with some specific decls that are not IP
invariant.
Richard.
>
> Thanks,
> Andrew Pinski
>
>
> >
> > Richard.
> >
> > > +
> > > /* It is preferable to swap two SSA_NAME to ensure a canonical form
> > > for commutative and comparison operators. Ensuring a canonical
> > > form allows the optimizers to find additional redundancies without
> > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr118902-1.c
> > > b/gcc/testsuite/gcc.dg/tree-ssa/pr118902-1.c
> > > new file mode 100644
> > > index 00000000000..fa21b8a74ef
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr118902-1.c
> > > @@ -0,0 +1,21 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-O2 -fdump-tree-optimized" } */
> > > +
> > > +void foo(int);
> > > +void l(int**);
> > > +int f1(int j, int t)
> > > +{
> > > + int p = 0;
> > > + int *a = &p;
> > > + l(&a);
> > > + if (a == &p)
> > > + return 0;
> > > + for(int i = 0; i < j; i++)
> > > + {
> > > + if (a == &p) foo(p);
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +/* We should be able to remove the call to foo because a is never equal
> > > to &p inside the loop. */
> > > +/* { dg-final { scan-tree-dump-not "foo " "optimized"} } */
> > > --
> > > 2.43.0
> > >
