On Tue, Jul 7, 2020 at 6:03 AM Hans-Peter Nilsson via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The store to the whole of each volatile object was picked apart
> like there had been an individual assignment to each of the
> fields. Reads were added as part of that; see PR for details.
> The reads from volatile memory were a clear bug; individual
> stores questionable. A separate patch clarifies the docs.
>
> Tested x86_64-linux, powerpc64le-linux and cris-elf.
> Ok to commit? Backport to gcc-10?
OK for both.
Thanks,
Richard.
> gcc:
> PR middle-end/94600
> * expr.c (expand_constructor): Make a temporary also if we're
> storing to volatile memory.
>
> gcc/testsuite:
> PR middle-end/94600
> * gcc.dg/pr94600-1.c, gcc.dg/pr94600-2.c, gcc.dg/pr94600-3.c,
> gcc.dg/pr94600-4.c, gcc.dg/pr94600-5.c, gcc.dg/pr94600-6.c,
> gcc.dg/pr94600-7.c, gcc.dg/pr94600-8.c: New tests.
> ---
> gcc/expr.c | 5 ++++-
> gcc/testsuite/gcc.dg/pr94600-1.c | 36 ++++++++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/pr94600-2.c | 34 ++++++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/pr94600-3.c | 35 +++++++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/pr94600-4.c | 34 ++++++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/pr94600-5.c | 34 ++++++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/pr94600-6.c | 33 +++++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/pr94600-7.c | 33 +++++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/pr94600-8.c | 33 +++++++++++++++++++++++++++++++++
> 9 files changed, 276 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr94600-1.c
> create mode 100644 gcc/testsuite/gcc.dg/pr94600-2.c
> create mode 100644 gcc/testsuite/gcc.dg/pr94600-3.c
> create mode 100644 gcc/testsuite/gcc.dg/pr94600-4.c
> create mode 100644 gcc/testsuite/gcc.dg/pr94600-5.c
> create mode 100644 gcc/testsuite/gcc.dg/pr94600-6.c
> create mode 100644 gcc/testsuite/gcc.dg/pr94600-7.c
> create mode 100644 gcc/testsuite/gcc.dg/pr94600-8.c
>
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 3c68b0d754c..44ea577e03d 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -8379,7 +8379,10 @@ expand_constructor (tree exp, rtx target, enum
> expand_modifier modifier,
> /* Handle calls that pass values in multiple non-contiguous
> locations. The Irix 6 ABI has examples of this. */
> if (target == 0 || ! safe_from_p (target, exp, 1)
> - || GET_CODE (target) == PARALLEL || modifier == EXPAND_STACK_PARM)
> + || GET_CODE (target) == PARALLEL || modifier == EXPAND_STACK_PARM
> + /* Also make a temporary if the store is to volatile memory, to
> + avoid individual accesses to aggregate members. */
> + || (GET_CODE (target) == MEM && MEM_VOLATILE_P (target)))
> {
> if (avoid_temp_mem)
> return NULL_RTX;
> diff --git a/gcc/testsuite/gcc.dg/pr94600-1.c
> b/gcc/testsuite/gcc.dg/pr94600-1.c
> new file mode 100644
> index 00000000000..b5913a0939c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94600-1.c
> @@ -0,0 +1,36 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target size32plus } */
> +/* { dg-options "-fdump-rtl-final -O2" } */
> +
> +/* Assignments to a whole struct of suitable size (32 bytes) must not be
> + picked apart into field accesses. */
> +
> +typedef struct {
> + unsigned int f0 : 4;
> + unsigned int f1 : 11;
> + unsigned int f2 : 10;
> + unsigned int f3 : 7;
> +} t0;
> +
> +static t0 a0[] = {
> + { .f0 = 7, .f1 = 99, .f3 = 1, },
> + { .f0 = 7, .f1 = 251, .f3 = 1, },
> + { .f0 = 8, .f1 = 127, .f3 = 5, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> +};
> +
> +void
> +foo(void)
> +{
> + __SIZE_TYPE__ i;
> + __SIZE_TYPE__ base = 0x000a0000;
> + for (i = 0; i < (sizeof (a0) / sizeof ((a0)[0])); i++) {
> + *(volatile t0 *) (base + 44 + i * 4) = a0[i];
> + }
> +}
> +
> +/* The only volatile accesses should be the obvious writes. */
> +/* { dg-final { scan-rtl-dump-times {\(mem/v} 6 "final" } } */
> +/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 6 "final" } } */
> diff --git a/gcc/testsuite/gcc.dg/pr94600-2.c
> b/gcc/testsuite/gcc.dg/pr94600-2.c
> new file mode 100644
> index 00000000000..cb96cc98a2d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94600-2.c
> @@ -0,0 +1,34 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target size32plus } */
> +/* { dg-options "-fdump-rtl-final -O2" } */
> +
> +/* Unrolled version of pr94600-1.c. */
> +
> +typedef struct {
> + unsigned int f0 : 4;
> + unsigned int f1 : 11;
> + unsigned int f2 : 10;
> + unsigned int f3 : 7;
> +} t0;
> +
> +void
> +bar(void)
> +{
> + t0 a00 = { .f0 = 7, .f1 = 99, .f3 = 1, };
> + t0 a01 = { .f0 = 7, .f1 = 251, .f3 = 1, };
> + t0 a02 = { .f0 = 8, .f1 = 127, .f3 = 5, };
> + t0 a03 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + t0 a04 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + t0 a05 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + __SIZE_TYPE__ base = 0x000a0000;
> +
> + *(volatile t0 *) ((base) + 44 + (0) * 4) = a00;
> + *(volatile t0 *) ((base) + 44 + (1) * 4) = a01;
> + *(volatile t0 *) ((base) + 44 + (2) * 4) = a02;
> + *(volatile t0 *) ((base) + 44 + (3) * 4) = a03;
> + *(volatile t0 *) ((base) + 44 + (4) * 4) = a04;
> + *(volatile t0 *) ((base) + 44 + (5) * 4) = a05;
> +}
> +
> +/* { dg-final { scan-rtl-dump-times {\(mem/v} 6 "final" } } */
> +/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 6 "final" } } */
> diff --git a/gcc/testsuite/gcc.dg/pr94600-3.c
> b/gcc/testsuite/gcc.dg/pr94600-3.c
> new file mode 100644
> index 00000000000..7537f6cb797
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94600-3.c
> @@ -0,0 +1,35 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target size32plus } */
> +/* { dg-options "-fdump-rtl-final -O2 -fno-unroll-loops" } */
> +
> +/* Same-address version of pr94600-1.c. */
> +
> +typedef struct {
> + unsigned int f0 : 4;
> + unsigned int f1 : 11;
> + unsigned int f2 : 10;
> + unsigned int f3 : 7;
> +} t0;
> +
> +static t0 a0[] = {
> + { .f0 = 7, .f1 = 99, .f3 = 1, },
> + { .f0 = 7, .f1 = 251, .f3 = 1, },
> + { .f0 = 8, .f1 = 127, .f3 = 5, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> +};
> +
> +void
> +foo(void)
> +{
> + __SIZE_TYPE__ i;
> + __SIZE_TYPE__ base = 0x000a0000;
> + for (i = 0; i < (sizeof (a0) / sizeof ((a0)[0])); i++) {
> + *(volatile t0 *) (base + 44) = a0[i];
> + }
> +}
> +
> +/* The loop isn't unrolled. */
> +/* { dg-final { scan-rtl-dump-times {\(mem/v} 1 "final" } } */
> +/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 1 "final" } } */
> diff --git a/gcc/testsuite/gcc.dg/pr94600-4.c
> b/gcc/testsuite/gcc.dg/pr94600-4.c
> new file mode 100644
> index 00000000000..c2901abb327
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94600-4.c
> @@ -0,0 +1,34 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target size32plus } */
> +/* { dg-options "-fdump-rtl-final -O2" } */
> +
> +/* Unrolled version of pr94600-2.c. */
> +
> +typedef struct {
> + unsigned int f0 : 4;
> + unsigned int f1 : 11;
> + unsigned int f2 : 10;
> + unsigned int f3 : 7;
> +} t0;
> +
> +void
> +bar(void)
> +{
> + t0 a00 = { .f0 = 7, .f1 = 99, .f3 = 1, };
> + t0 a01 = { .f0 = 7, .f1 = 251, .f3 = 1, };
> + t0 a02 = { .f0 = 8, .f1 = 127, .f3 = 5, };
> + t0 a03 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + t0 a04 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + t0 a05 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + __SIZE_TYPE__ base = 0x000a0000;
> +
> + *(volatile t0 *) ((base) + 44) = a00;
> + *(volatile t0 *) ((base) + 44) = a01;
> + *(volatile t0 *) ((base) + 44) = a02;
> + *(volatile t0 *) ((base) + 44) = a03;
> + *(volatile t0 *) ((base) + 44) = a04;
> + *(volatile t0 *) ((base) + 44) = a05;
> +}
> +
> +/* { dg-final { scan-rtl-dump-times {\(mem/v} 6 "final" } } */
> +/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 6 "final" } } */
> diff --git a/gcc/testsuite/gcc.dg/pr94600-5.c
> b/gcc/testsuite/gcc.dg/pr94600-5.c
> new file mode 100644
> index 00000000000..90085b3b1df
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94600-5.c
> @@ -0,0 +1,34 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target size32plus } */
> +/* { dg-options "-fdump-rtl-final -O2 -fno-unroll-loops" } */
> +
> +/* Target-as-parameter version of pr94600-1.c. */
> +
> +typedef struct {
> + unsigned int f0 : 4;
> + unsigned int f1 : 11;
> + unsigned int f2 : 10;
> + unsigned int f3 : 7;
> +} t0;
> +
> +static t0 a0[] = {
> + { .f0 = 7, .f1 = 99, .f3 = 1, },
> + { .f0 = 7, .f1 = 251, .f3 = 1, },
> + { .f0 = 8, .f1 = 127, .f3 = 5, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> +};
> +
> +void
> +foo(volatile t0 *b)
> +{
> + __SIZE_TYPE__ i;
> + for (i = 0; i < (sizeof (a0) / sizeof ((a0)[0])); i++) {
> + b[i+11] = a0[i];
> + }
> +}
> +
> +/* The loop isn't unrolled. */
> +/* { dg-final { scan-rtl-dump-times {\(mem/v} 1 "final" } } */
> +/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 1 "final" } } */
> diff --git a/gcc/testsuite/gcc.dg/pr94600-6.c
> b/gcc/testsuite/gcc.dg/pr94600-6.c
> new file mode 100644
> index 00000000000..23a81a01f49
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94600-6.c
> @@ -0,0 +1,33 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target size32plus } */
> +/* { dg-options "-fdump-rtl-final -O2" } */
> +
> +/* Target-as-parameter version of pr94600-2.c. */
> +
> +typedef struct {
> + unsigned int f0 : 4;
> + unsigned int f1 : 11;
> + unsigned int f2 : 10;
> + unsigned int f3 : 7;
> +} t0;
> +
> +void
> +bar(volatile t0 *b)
> +{
> + t0 a00 = { .f0 = 7, .f1 = 99, .f3 = 1, };
> + t0 a01 = { .f0 = 7, .f1 = 251, .f3 = 1, };
> + t0 a02 = { .f0 = 8, .f1 = 127, .f3 = 5, };
> + t0 a03 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + t0 a04 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + t0 a05 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> +
> + b[11+0] = a00;
> + b[11+1] = a01;
> + b[11+2] = a02;
> + b[11+3] = a03;
> + b[11+4] = a04;
> + b[11+5] = a05;
> +}
> +
> +/* { dg-final { scan-rtl-dump-times {\(mem/v} 6 "final" } } */
> +/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 6 "final" } } */
> diff --git a/gcc/testsuite/gcc.dg/pr94600-7.c
> b/gcc/testsuite/gcc.dg/pr94600-7.c
> new file mode 100644
> index 00000000000..2f5c759d3a1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94600-7.c
> @@ -0,0 +1,33 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target size32plus } */
> +/* { dg-options "-fdump-rtl-final -O2 -fno-unroll-loops" } */
> +
> +/* Target-as-parameter version of pr94600-3.c. */
> +
> +typedef struct {
> + unsigned int f0 : 4;
> + unsigned int f1 : 11;
> + unsigned int f2 : 10;
> + unsigned int f3 : 7;
> +} t0;
> +
> +static t0 a0[] = {
> + { .f0 = 7, .f1 = 99, .f3 = 1, },
> + { .f0 = 7, .f1 = 251, .f3 = 1, },
> + { .f0 = 8, .f1 = 127, .f3 = 5, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> + { .f0 = 5, .f1 = 1, .f3 = 1, },
> +};
> +
> +void
> +foo(volatile t0 *b)
> +{
> + __SIZE_TYPE__ i;
> + for (i = 0; i < (sizeof (a0) / sizeof ((a0)[0])); i++) {
> + b[11] = a0[i];
> + }
> +}
> +
> +/* { dg-final { scan-rtl-dump-times {\(mem/v} 1 "final" } } */
> +/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 1 "final" } } */
> diff --git a/gcc/testsuite/gcc.dg/pr94600-8.c
> b/gcc/testsuite/gcc.dg/pr94600-8.c
> new file mode 100644
> index 00000000000..ded814b3b95
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr94600-8.c
> @@ -0,0 +1,33 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target size32plus } */
> +/* { dg-options "-fdump-rtl-final -O2" } */
> +
> +/* Unrolled version of pr94600-2.c. */
> +
> +typedef struct {
> + unsigned int f0 : 4;
> + unsigned int f1 : 11;
> + unsigned int f2 : 10;
> + unsigned int f3 : 7;
> +} t0;
> +
> +void
> +bar(volatile t0 *b)
> +{
> + t0 a00 = { .f0 = 7, .f1 = 99, .f3 = 1, };
> + t0 a01 = { .f0 = 7, .f1 = 251, .f3 = 1, };
> + t0 a02 = { .f0 = 8, .f1 = 127, .f3 = 5, };
> + t0 a03 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + t0 a04 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> + t0 a05 = { .f0 = 5, .f1 = 1, .f3 = 1, };
> +
> + b[11] = a00;
> + b[11] = a01;
> + b[11] = a02;
> + b[11] = a03;
> + b[11] = a04;
> + b[11] = a05;
> +}
> +
> +/* { dg-final { scan-rtl-dump-times {\(mem/v} 6 "final" } } */
> +/* { dg-final { scan-rtl-dump-times {\(set \(mem/v} 6 "final" } } */
> --
> 2.11.0
>
