This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix SRA with volatile loads/stores (PR tree-optimization/58145)
- From: Richard Biener <rguenther at suse dot de>
- To: Jakub Jelinek <jakub at redhat dot com>, Jakub Jelinek <jakub at redhat dot com>, Martin Jambor <mjambor at suse dot cz>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Wed, 14 Aug 2013 19:53:31 +0200
- Subject: Re: [PATCH] Fix SRA with volatile loads/stores (PR tree-optimization/58145)
- References: <20130814130922 dot GO1814 at tucnak dot redhat dot com>
Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>On the following testcases we miscompile the code (on -1.c just drop
>= {v} from the statements, on -2.c lim moves the volatile stores after
>the
>loop), because SRA drops the volatileness from the MEM_REF.
>
>SRA generally ignores volatile vars and fields etc., but if we have a
>structure assignment to volatile from non-volatile or vice versa,
>if SRA decides to scalarize rhs resp. lhs, new MEM_REFs are created
>even
>for the volatile access with different type.
>
>The following patch fixes that by propagating TREE_THIS_VOLATILE and
>TREE_SIDE_EFFECTS from the prev_base to the newly created MEM_REF.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>trunk/4.8?
You do not need the volatile qualified type, and IIRC we're not consistent in setting tree-sideeffects either.
Thus,
Ok with not doing the volatile qualification.
Thanks,
Richard.
>On IRC with Martin we've also discussed slsr in this regard, but that
>seems
>to be fine, it uses the original volatile type if it was volatile and
>propagates TREE_THIS_VOLATILE/TREE_SIDE_EFFECTS flags to the newly
>created
>MEM_REF.
>
>2013-08-14 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/58145
> * tree-sra.c (build_ref_for_offset): If base is TREE_THIS_VOLATILE,
> propagate it to exp_type and MEM_REF.
>
> * gcc.dg/pr58145-1.c: New test.
> * gcc.dg/pr58145-2.c: New test.
>
>--- gcc/tree-sra.c.jj 2013-08-14 11:02:55.290711106 +0200
>+++ gcc/tree-sra.c 2013-08-14 12:38:47.405230042 +0200
>@@ -1466,6 +1466,7 @@ build_ref_for_offset (location_t loc, tr
> {
> tree prev_base = base;
> tree off;
>+ tree mem_ref;
> HOST_WIDE_INT base_offset;
> unsigned HOST_WIDE_INT misalign;
> unsigned int align;
>@@ -1515,8 +1516,17 @@ build_ref_for_offset (location_t loc, tr
> align = (misalign & -misalign);
> if (align < TYPE_ALIGN (exp_type))
> exp_type = build_aligned_type (exp_type, align);
>-
>- return fold_build2_loc (loc, MEM_REF, exp_type, base, off);
>+ if (TREE_THIS_VOLATILE (TREE_TYPE (prev_base))
>+ && !TREE_THIS_VOLATILE (exp_type))
>+ exp_type = build_qualified_type (exp_type, TYPE_QUALS (exp_type)
>+ | TYPE_QUAL_VOLATILE);
>+
>+ mem_ref = fold_build2_loc (loc, MEM_REF, exp_type, base, off);
>+ if (TREE_THIS_VOLATILE (exp_type) || TREE_THIS_VOLATILE (prev_base))
>+ TREE_THIS_VOLATILE (mem_ref) = 1;
>+ if (TREE_SIDE_EFFECTS (prev_base))
>+ TREE_SIDE_EFFECTS (mem_ref) = 1;
>+ return mem_ref;
> }
>
>/* Construct a memory reference to a part of an aggregate BASE at the
>given
>--- gcc/testsuite/gcc.dg/pr58145-1.c.jj 2013-08-14 12:02:07.077086488
>+0200
>+++ gcc/testsuite/gcc.dg/pr58145-1.c 2013-08-14 12:03:15.895198976
>+0200
>@@ -0,0 +1,37 @@
>+/* PR tree-optimization/58145 */
>+/* { dg-do compile { target { int32plus } } } */
>+/* { dg-options "-O2 -fdump-tree-optimized" } */
>+
>+struct S { unsigned int data : 32; };
>+struct T { unsigned int data; };
>+volatile struct S s2;
>+
>+void
>+f1 (int val)
>+{
>+ struct S s = { .data = val };
>+ *(volatile struct S *) 0x880000UL = s;
>+}
>+
>+void
>+f2 (int val)
>+{
>+ struct T t = { .data = val };
>+ *(volatile struct T *) 0x880000UL = t;
>+}
>+
>+void
>+f3 (int val)
>+{
>+ *(volatile unsigned int *) 0x880000UL = val;
>+}
>+
>+void
>+f4 (int val)
>+{
>+ struct S s = { .data = val };
>+ s2 = s;
>+}
>+
>+/* { dg-final { scan-tree-dump-times " ={v} " 4 "optimized" } } */
>+/* { dg-final { cleanup-tree-dump "optimized" } } */
>--- gcc/testsuite/gcc.dg/pr58145-2.c.jj 2013-08-14 12:02:28.409663559
>+0200
>+++ gcc/testsuite/gcc.dg/pr58145-2.c 2013-08-14 12:04:19.471612107
>+0200
>@@ -0,0 +1,51 @@
>+/* PR tree-optimization/58145 */
>+/* { dg-do compile { target { int32plus } } } */
>+/* { dg-options "-O2 -fdump-tree-optimized" } */
>+
>+struct S { unsigned int data : 32; };
>+struct T { unsigned int data; };
>+volatile struct S s2;
>+
>+static inline void
>+f1 (int val)
>+{
>+ struct S s = { .data = val };
>+ *(volatile struct S *) 0x880000UL = s;
>+}
>+
>+static inline void
>+f2 (int val)
>+{
>+ struct T t = { .data = val };
>+ *(volatile struct T *) 0x880000UL = t;
>+}
>+
>+static inline void
>+f3 (int val)
>+{
>+ *(volatile unsigned int *) 0x880000UL = val;
>+}
>+
>+static inline void
>+f4 (int val)
>+{
>+ struct S s = { .data = val };
>+ s2 = s;
>+}
>+
>+void
>+f5 (void)
>+{
>+ int i;
>+ for (i = 0; i < 100; i++)
>+ f1 (0);
>+ for (i = 0; i < 100; i++)
>+ f2 (0);
>+ for (i = 0; i < 100; i++)
>+ f3 (0);
>+ for (i = 0; i < 100; i++)
>+ f4 (0);
>+}
>+
>+/* { dg-final { scan-tree-dump-times " ={v} " 4 "optimized" } } */
>+/* { dg-final { cleanup-tree-dump "optimized" } } */
>
> Jakub