Patchwork Fix SRA with volatile loads/stores (PR tree-optimization/58145)

login
register
mail settings
Submitter Jakub Jelinek
Date Aug. 14, 2013, 1:09 p.m.
Message ID <20130814130922.GO1814@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/267117/
State New
Headers show

Comments

Jakub Jelinek - Aug. 14, 2013, 1:09 p.m.
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?

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.


	Jakub
Richard Guenther - Aug. 14, 2013, 5:53 p.m.
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

Patch

--- 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" } } */