* [PATCH 1/2] PR94600: fix volatile access to the whole of a compound object.
@ 2020-07-07 4:01 Hans-Peter Nilsson
2020-07-07 4:08 ` Hans-Peter Nilsson
2020-07-07 7:04 ` Richard Biener
0 siblings, 2 replies; 5+ messages in thread
From: Hans-Peter Nilsson @ 2020-07-07 4:01 UTC (permalink / raw)
To: gcc-patches
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?
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 1/2] PR94600: fix volatile access to the whole of a compound object.
2020-07-07 4:01 [PATCH 1/2] PR94600: fix volatile access to the whole of a compound object Hans-Peter Nilsson
@ 2020-07-07 4:08 ` Hans-Peter Nilsson
2020-07-07 7:04 ` Richard Biener
1 sibling, 0 replies; 5+ messages in thread
From: Hans-Peter Nilsson @ 2020-07-07 4:08 UTC (permalink / raw)
To: gcc-patches
> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Tue, 7 Jul 2020 06:01:37 +0200
> gcc:
> PR middle-end/94600
> * expr.c (expand_constructor): Make a temporary also if we're
> storing to volatile memory.
Oops, I dropped attribution here, but this patch is by Richard
Biener (modulo the comment, that's my fault). See the PR.
brgds, H-P
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] PR94600: fix volatile access to the whole of a compound object.
2020-07-07 4:01 [PATCH 1/2] PR94600: fix volatile access to the whole of a compound object Hans-Peter Nilsson
2020-07-07 4:08 ` Hans-Peter Nilsson
@ 2020-07-07 7:04 ` Richard Biener
2020-07-14 13:37 ` [PATCH] expr: Unbreak build of mesa [PR96194] Jakub Jelinek
1 sibling, 1 reply; 5+ messages in thread
From: Richard Biener @ 2020-07-07 7:04 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: GCC Patches
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] expr: Unbreak build of mesa [PR96194]
2020-07-07 7:04 ` Richard Biener
@ 2020-07-14 13:37 ` Jakub Jelinek
2020-07-14 13:57 ` Richard Biener
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2020-07-14 13:37 UTC (permalink / raw)
To: Richard Biener; +Cc: Hans-Peter Nilsson, GCC Patches
Hi!
On Tue, Jul 07, 2020 at 09:04:40AM +0200, Richard Biener via Gcc-patches 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.
This breaks building of mesa on both the trunk and 10 branch.
The problem is that the middle-end may never create temporaries of non-POD
(TREE_ADDRESSABLE) types, those can be only created when the language says
so and thus only the FE is allowed to create those.
This patch just reverts the behavior to what we used to do before for the
stores to volatile non-PODs. Perhaps we want to do something else, but
definitely we can't create temporaries of the non-POD type. It is up to
discussions on what should happen in those cases.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/10.2?
2020-07-14 Jakub Jelinek <jakub@redhat.com>
PR middle-end/96194
* expr.c (expand_constructor): Don't create temporary for store to
volatile MEM if exp has an addressable type.
* g++.dg/opt/pr96194.C: New test.
--- gcc/expr.c.jj 2020-07-13 19:09:33.173872178 +0200
+++ gcc/expr.c 2020-07-14 13:07:26.228801996 +0200
@@ -8382,7 +8382,9 @@ expand_constructor (tree exp, rtx target
|| 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)))
+ || (GET_CODE (target) == MEM
+ && MEM_VOLATILE_P (target)
+ && !TREE_ADDRESSABLE (TREE_TYPE (exp))))
{
if (avoid_temp_mem)
return NULL_RTX;
--- gcc/testsuite/g++.dg/opt/pr96194.C.jj 2020-07-14 13:15:32.185733939 +0200
+++ gcc/testsuite/g++.dg/opt/pr96194.C 2020-07-14 13:16:16.033096197 +0200
@@ -0,0 +1,21 @@
+// PR middle-end/96194
+// { dg-do compile }
+// { dg-options "-O2" }
+
+#include <new>
+
+struct A { ~A (); };
+struct B : A { float e[64]; };
+
+B *
+foo ()
+{
+ return new ((void *) 0) B ();
+}
+
+B *
+bar (void *x, bool y)
+{
+ void *p = y ? x : (void *) 0;
+ return new (p) B ();
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] expr: Unbreak build of mesa [PR96194]
2020-07-14 13:37 ` [PATCH] expr: Unbreak build of mesa [PR96194] Jakub Jelinek
@ 2020-07-14 13:57 ` Richard Biener
0 siblings, 0 replies; 5+ messages in thread
From: Richard Biener @ 2020-07-14 13:57 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Hans-Peter Nilsson, GCC Patches
On July 14, 2020 3:37:35 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>On Tue, Jul 07, 2020 at 09:04:40AM +0200, Richard Biener via
>Gcc-patches 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.
>
>This breaks building of mesa on both the trunk and 10 branch.
>
>The problem is that the middle-end may never create temporaries of
>non-POD
>(TREE_ADDRESSABLE) types, those can be only created when the language
>says
>so and thus only the FE is allowed to create those.
>
>This patch just reverts the behavior to what we used to do before for
>the
>stores to volatile non-PODs. Perhaps we want to do something else, but
>definitely we can't create temporaries of the non-POD type. It is up
>to
>discussions on what should happen in those cases.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>trunk/10.2?
OK.
Richard.
>2020-07-14 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/96194
> * expr.c (expand_constructor): Don't create temporary for store to
> volatile MEM if exp has an addressable type.
>
> * g++.dg/opt/pr96194.C: New test.
>
>--- gcc/expr.c.jj 2020-07-13 19:09:33.173872178 +0200
>+++ gcc/expr.c 2020-07-14 13:07:26.228801996 +0200
>@@ -8382,7 +8382,9 @@ expand_constructor (tree exp, rtx target
> || 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)))
>+ || (GET_CODE (target) == MEM
>+ && MEM_VOLATILE_P (target)
>+ && !TREE_ADDRESSABLE (TREE_TYPE (exp))))
> {
> if (avoid_temp_mem)
> return NULL_RTX;
>--- gcc/testsuite/g++.dg/opt/pr96194.C.jj 2020-07-14 13:15:32.185733939
>+0200
>+++ gcc/testsuite/g++.dg/opt/pr96194.C 2020-07-14 13:16:16.033096197
>+0200
>@@ -0,0 +1,21 @@
>+// PR middle-end/96194
>+// { dg-do compile }
>+// { dg-options "-O2" }
>+
>+#include <new>
>+
>+struct A { ~A (); };
>+struct B : A { float e[64]; };
>+
>+B *
>+foo ()
>+{
>+ return new ((void *) 0) B ();
>+}
>+
>+B *
>+bar (void *x, bool y)
>+{
>+ void *p = y ? x : (void *) 0;
>+ return new (p) B ();
>+}
>
>
> Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-14 13:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 4:01 [PATCH 1/2] PR94600: fix volatile access to the whole of a compound object Hans-Peter Nilsson
2020-07-07 4:08 ` Hans-Peter Nilsson
2020-07-07 7:04 ` Richard Biener
2020-07-14 13:37 ` [PATCH] expr: Unbreak build of mesa [PR96194] Jakub Jelinek
2020-07-14 13:57 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).