On Mon, Apr 21, 2025 at 1:42 AM Richard Biener <richard.guent...@gmail.com> wrote: > > On Thu, Apr 17, 2025 at 7:37 PM Andrew Pinski <quic_apin...@quicinc.com> > 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 <quic_apin...@quicinc.com> > > --- > > 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/pine.lnx.4.64.0702161440530.18...@zhemvz.fhfr.qr/ I do wonder if we need a better way of describing the difference between gimple and generic too. > > 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. 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 > >
