From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x642.google.com (mail-ej1-x642.google.com [IPv6:2a00:1450:4864:20::642]) by sourceware.org (Postfix) with ESMTPS id 4776A387088B for ; Tue, 7 Jul 2020 07:04:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4776A387088B Received: by mail-ej1-x642.google.com with SMTP id dp18so45492535ejc.8 for ; Tue, 07 Jul 2020 00:04:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=bcDLoeWKRk3LMvFuuD+t6wkU+CSQYIp+K5HF+jJ+1cc=; b=ALObpo+cfjlAZC8G7gad8PcwPW4OeKupNUxOK3ykM5meud/GUisNRW7+4g1P91ddjt I2j3mce4Gx3PSlIA2cFAJn3e7mcXBp6FqcFyj0bdNAvK2Ni/uJit59TcyjVXVQeNdhTc X6rJFJjXOiBuEq9RXgz16uPB8LPsveTM3XRpnAiKDWMo6/HtB082WPc60A5J3WnZQV7i zS3SPnSYFuJgkPS3kf4M0j3wsx1rc92qm6zeZMKLRYDGjC0umWMuBePplxEv1MWVjAg6 JyZM5peVeVtPtBMLK94j2OpxrRr1b1XwGsXsGz6JUOFtuQ0vCzsPSBuUHzE2WTjJ7on6 c6TQ== X-Gm-Message-State: AOAM531YVEffcKX1fBdpTpq5558hepNQH0jw0+FAVnx0lFLeMapBezwW c6Fvzsa8jSZgNWPp6sZRldpd5xI58N23BHISrVw= X-Google-Smtp-Source: ABdhPJzVCoiqYZDZP5XdcpLXWe68Rsyd9lohrTSB2+sVxhwr+CVSYajL2/rAunz+Z7RtYxFzUKFv6VilWl2iPJ99fVI= X-Received: by 2002:a17:906:17c8:: with SMTP id u8mr47104481eje.129.1594105492226; Tue, 07 Jul 2020 00:04:52 -0700 (PDT) MIME-Version: 1.0 References: <202007070401.06741bpX024052@ignucius.se.axis.com> In-Reply-To: <202007070401.06741bpX024052@ignucius.se.axis.com> From: Richard Biener Date: Tue, 7 Jul 2020 09:04:40 +0200 Message-ID: Subject: Re: [PATCH 1/2] PR94600: fix volatile access to the whole of a compound object. To: Hans-Peter Nilsson Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Jul 2020 07:04:55 -0000 On Tue, Jul 7, 2020 at 6:03 AM Hans-Peter Nilsson 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. 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 >