From: Richard Biener <richard.guenther@gmail.com>
To: Hans-Peter Nilsson <hp@axis.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH 1/2] PR94600: fix volatile access to the whole of a compound object.
Date: Tue, 7 Jul 2020 09:04:40 +0200 [thread overview]
Message-ID: <CAFiYyc0-0fbGE2BtZm9SyouC+=5Ei4pTWZD3kjGMozaGy799=A@mail.gmail.com> (raw)
In-Reply-To: <202007070401.06741bpX024052@ignucius.se.axis.com>
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
>
next prev parent reply other threads:[~2020-07-07 7:04 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-07 4:01 Hans-Peter Nilsson
2020-07-07 4:08 ` Hans-Peter Nilsson
2020-07-07 7:04 ` Richard Biener [this message]
2020-07-14 13:37 ` [PATCH] expr: Unbreak build of mesa [PR96194] Jakub Jelinek
2020-07-14 13:57 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc0-0fbGE2BtZm9SyouC+=5Ei4pTWZD3kjGMozaGy799=A@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hp@axis.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).