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