cfgexpand: Update partition size when merging variables

cfgexpand sorts variables by decreasing size, so when merging a later
variable into an earlier one, there's usually no need to update the
merged size.

But for poly_int sizes, the sort function just uses a lexicographical
comparison of the coefficients, so e.g. 2X+2 comes before 0X+32.
Which is bigger depends on the runtime value of X.

This patch therefore takes the upper bound of the two sizes, which
is conservatively correct for variable-length vectors and a no-op
on other targets.

It's probably a bad idea to merge fixed-length and variable-length
variables in practice, but that's really an optimisation decision.
I think we should have this patch as a correctness fix either way.

This is easiest to test using the ACLE, but in principle it could happen
for autovectorised code too, e.g. when using OpenMP vector variables.
It's therefore a regression from GCC 8.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Richard


2020-01-22  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* cfgexpand.c (union_stack_vars): Update the size.

gcc/testsuite/
	* gcc.target/aarch64/sve/acle/general/stack_vars_1.c: New test.
---
 gcc/cfgexpand.c                               |  3 ++
 .../aarch64/sve/acle/general/stack_vars_1.c   | 32 +++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/acle/general/stack_vars_1.c

On Wed, Jan 22, 2020 at 11:59 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> cfgexpand sorts variables by decreasing size, so when merging a later
> variable into an earlier one, there's usually no need to update the
> merged size.
>
> But for poly_int sizes, the sort function just uses a lexicographical
> comparison of the coefficients, so e.g. 2X+2 comes before 0X+32.
> Which is bigger depends on the runtime value of X.
>
> This patch therefore takes the upper bound of the two sizes, which
> is conservatively correct for variable-length vectors and a no-op
> on other targets.
>
> It's probably a bad idea to merge fixed-length and variable-length
> variables in practice, but that's really an optimisation decision.
> I think we should have this patch as a correctness fix either way.
>
> This is easiest to test using the ACLE, but in principle it could happen
> for autovectorised code too, e.g. when using OpenMP vector variables.
> It's therefore a regression from GCC 8.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

OK.

Richard.

> Richard
>
>
> 2020-01-22  Richard Sandiford  <richard.sandiford@arm.com>
>
> gcc/
>         * cfgexpand.c (union_stack_vars): Update the size.
>
> gcc/testsuite/
>         * gcc.target/aarch64/sve/acle/general/stack_vars_1.c: New test.
> ---
>  gcc/cfgexpand.c                               |  3 ++
>  .../aarch64/sve/acle/general/stack_vars_1.c   | 32 +++++++++++++++++++
>  2 files changed, 35 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/acle/general/stack_vars_1.c
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index fb0575b146e..9864e4344d2 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -862,6 +862,9 @@ union_stack_vars (size_t a, size_t b)
>    stack_vars[b].representative = a;
>    stack_vars[a].next = b;
>
> +  /* Make sure A is big enough to hold B.  */
> +  stack_vars[a].size = upper_bound (stack_vars[a].size, stack_vars[b].size);
> +
>    /* Update the required alignment of partition A to account for B.  */
>    if (stack_vars[a].alignb < stack_vars[b].alignb)
>      stack_vars[a].alignb = stack_vars[b].alignb;
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/stack_vars_1.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/stack_vars_1.c
> new file mode 100644
> index 00000000000..860fa67fbe6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/stack_vars_1.c
> @@ -0,0 +1,32 @@
> +/* { dg-do run { target aarch64_sve_hw } } */
> +/* { dg-additional-options "-O2" } */
> +
> +#include <arm_sve.h>
> +
> +struct s { int x[32]; };
> +
> +void __attribute__((noipa)) consume (void *ptr) {}
> +
> +void __attribute__((noipa))
> +check_var (svint32_t *ptr)
> +{
> +  svbool_t pg = svptrue_b8 ();
> +  if (svptest_any (pg, svcmpne (pg, *ptr, svindex_s32 (0, 1))))
> +    __builtin_abort ();
> +}
> +
> +int
> +main (void)
> +{
> +  svint32_t res = svindex_s32 (0, 1);
> +  {
> +    __SVBool_t pg = svptrue_b8 ();
> +    consume (&pg);
> +  }
> +  {
> +    struct s zeros = { 0 };
> +    consume (&zeros);
> +  }
> +  check_var (&res);
> +  return 0;
> +}