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).
> 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
> > >
