On Mon, May 12, 2025 at 3:53 AM Richard Biener <richard.guent...@gmail.com> wrote: > > On Fri, May 9, 2025 at 10:12 PM Andrew Pinski <pins...@gmail.com> wrote: > > > > 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? > > > > > > 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. > > > > > > 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. > > > > Coming back to this, I will make a change to put ADDR first instead of > > my patch of is_gimple_invariant_address, next week. > > > > Note I just noticed while trying to remove > > forward_propagate_into_gimple_cond and > > forward_propagate_into_comparison that we have: > > (for cmp (eq ne) > > (simplify > > /* SSA names are canonicalized to 2nd place. */ > > (cmp addr@0 SSA_NAME@1) > > > > But that seems wrong if we had SSA_NAME which was defined by an > > ADDR_EXPR as we don't redo canonicalization when doing valueization. > > We always valueize first, then canonicalize, so this should work (but the > above would need adjustment if we order ADDR_EXPR differently now).
So doing ADDR_EXPR last worked and I submitted a patch which fixes up the issue with the already applied patch: https://gcc.gnu.org/pipermail/gcc-patches/2025-May/683711.html It includes the match change too as needed. Thanks, Andrew Pinski > > > It just happens to work in the end because fold will do the > > canonicalization before match and simplify and forwprop uses fold do > > the simplifcation during forward_propagate_into_comparison_1. While > > for match and simplify on gimple, it does not while valueization of > > the names. Anyways I will fix that; and add a comment on why it is not > > always canonicalized. Just bringing it up as a related issue here. > > > > 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 > > > >
