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