Wilco Dijkstra <wilco.dijks...@arm.com> writes:
> GCC tends to optimistically create CONST of globals with an immediate offset.
> However it is almost always better to CSE addresses of globals and add
> immediate
> offsets separately (the offset could be merged later in single-use cases).
> Splitting CONST expressions with an index in aarch64_legitimize_address fixes
> part
> of PR112573.
>
> Passes regress & bootstrap, OK for commit?
>
> gcc/ChangeLog:
> PR target/112573
> * config/aarch64/aarch64.cc (aarch64_legitimize_address): Reassociate
> badly
> formed CONST expressions.
>
> gcc/testsuite/ChangeLog:
> PR target/112573
> * gcc.target/aarch64/pr112573.c: Add new test.
>
> ---
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index
> 0909b319d16b9a1587314bcfda0a8112b42a663f..9fbc8b62455f48baec533d3dd5e2d9ea995d5a8f
> 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -12608,6 +12608,20 @@ aarch64_legitimize_address (rtx x, rtx /* orig_x
> */, machine_mode mode)
> not to split a CONST for some forms of address expression, otherwise
> it will generate sub-optimal code. */
>
> + /* First split X + CONST (base, offset) into (base + X) + offset. */
> + if (GET_CODE (x) == PLUS && GET_CODE (XEXP (x, 1)) == CONST)
> + {
> + poly_int64 offset;
> + rtx base = strip_offset_and_salt (XEXP (x, 1), &offset);
This should be just strip_offset, so that we don't lose the salt
during optimisation.
> +
> + if (offset.is_constant ())
I'm not sure this is really required. Logically the same thing
would apply to SVE, although admittedly:
/* { dg-do compile } */
/* { dg-options "-O2 -fno-section-anchors" } */
#include <arm_sve.h>
char a[2048];
void f1 (svint8_t x, int y)
{
*(svint8_t *)((a + y) + svcntb() * 3) = x;
*(svint8_t *)((a + y) + svcntb() * 2) = x;
*(svint8_t *)((a + y) + svcntb() * 1) = x;
*(svint8_t *)((a + y) + 0) = x;
}
/* { dg-final { scan-assembler-times "strb" 4 } } */
/* { dg-final { scan-assembler-times "adrp" 1 } } */
doesn't get arranged into the same form for other reasons (and already
produces somewhat decent code).
The patch is OK from my POV without the offset.is_constant check and
with s/strip_offset_and_salt/strip_offset/. Please say if there's
a reason to keep the offset check though.
Thanks,
Richard
> + {
> + base = expand_binop (Pmode, add_optab, base, XEXP (x, 0),
> + NULL_RTX, true, OPTAB_DIRECT);
> + x = plus_constant (Pmode, base, offset);
> + }
> + }
> +
> if (GET_CODE (x) == PLUS && CONST_INT_P (XEXP (x, 1)))
> {
> rtx base = XEXP (x, 0);
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr112573.c
> b/gcc/testsuite/gcc.target/aarch64/pr112573.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..be04c0ca86ad9f33975a85f497549955d6d1236d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr112573.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fno-section-anchors" } */
> +
> +char a[100];
> +
> +void f1 (int x, int y)
> +{
> + *((a + y) + 3) = x;
> + *((a + y) + 2) = x;
> + *((a + y) + 1) = x;
> + *((a + y) + 0) = x;
> +}
> +
> +/* { dg-final { scan-assembler-times "strb" 4 } } */
> +/* { dg-final { scan-assembler-times "adrp" 1 } } */
