public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [gimple] assignments to volatile
@ 2010-06-21 12:44 Nathan Sidwell
  2010-06-21 13:26 ` Richard Guenther
  0 siblings, 1 reply; 81+ messages in thread
From: Nathan Sidwell @ 2010-06-21 12:44 UTC (permalink / raw)
  To: GCC Patches

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

I've come across inconsistent and surprising behaviour when assigning to 
volatile lvalues.

Sometimes we re-read the assigned-to object, and sometimes we do not.  For instance,
    return vobj = data;
will cause a reread of vobj, IF data is not a constant.
or
   cond ? vobj = data : 0
will cause a reread of vobj, even when we don't use the result of the 
conditional expression -- unless data is a constant, in which case there is 
no-reread.  (And confusingly, if you put void casts inside the two conditional 
sub-expressions, the re-reading goes away, but it doesn't even when you 
explicitly cast the whole expression to void).

In the attached testcase, test_1, test_6 and test_7 have this surprising 
rereading behaviour.

The fault appears to be in gimplify_modify_expr, where we return the LHS as the 
value, if the caller wants the value.  The attached patch changes that routine 
when the target lvalue is volatile.  In that case it will create a temporary to 
hold the RHS value, assign that temporary to the LHS and then return the temporary.

Although the bug is architecture-neutral, the testcase is x86-specific because 
it's checking for specific accesses occurring.

built and tested on i686-pc-linux-gnu, ok?

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery


[-- Attachment #2: volatile.patch --]
[-- Type: text/x-patch, Size: 4198 bytes --]

2010-06-21  Nathan Sidwell  <nathan@codesourcery.com>

	gcc/
	* gimplify.c (gimplify_modify_expr): When assigning to volatiles,
	copy the src value and return a copy.

	gcc/testsuite/
	* gcc.target/i386/volatile-2.c: New.

Index: gimplify.c
===================================================================
--- gimplify.c	(revision 160876)
+++ gimplify.c	(working copy)
@@ -4467,6 +4467,28 @@
   gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
 	      || TREE_CODE (*expr_p) == INIT_EXPR);
 
+  if (want_value && TREE_THIS_VOLATILE (TREE_TYPE (*to_p)))
+    {
+      /* copy the rhs to a temporary, store the temporary and then
+         return it.  This makes sure we consistently do not re-read a
+         volatile just after storing it.  */
+      tree lhs = *to_p;
+      tree rhs = *from_p;
+      tree nv_type = TYPE_MAIN_VARIANT (TREE_TYPE (lhs));
+      tree tmp = create_tmp_var (nv_type, "modtmp");
+      tree tmp_ass = build2 (MODIFY_EXPR, nv_type, tmp, rhs);
+      tree vol_ass = build2 (MODIFY_EXPR, nv_type, lhs, tmp);
+      
+      recalculate_side_effects (tmp_ass);
+      gimplify_and_add (tmp_ass, pre_p);
+
+      recalculate_side_effects (vol_ass);
+      gimplify_and_add (vol_ass, pre_p);
+
+      *expr_p = tmp;
+      return GS_OK;
+    }
+  
   /* Insert pointer conversions required by the middle-end that are not
      required by the frontend.  This fixes middle-end type checking for
      for example gcc.dg/redecl-6.c.  */
Index: testsuite/gcc.target/i386/volatile-2.c
===================================================================
--- testsuite/gcc.target/i386/volatile-2.c	(revision 0)
+++ testsuite/gcc.target/i386/volatile-2.c	(revision 0)
@@ -0,0 +1,92 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/* Check volatiles are written, read or not re-read consistently */
+
+
+/* simple assignments */
+
+extern int volatile obj_0;
+void test_0 (int data)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_0" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_0," } } */
+  obj_0 = data;
+}
+
+extern int volatile obj_1;
+int test_1 (int data)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_1" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_1," } } */
+  return obj_1 = data;
+}
+
+extern int volatile obj_2;
+int test_2 (void)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_2" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_2," } } */
+  return obj_2 = 0;
+}
+
+
+/* Assignments in compound exprs */
+
+extern int volatile obj_3;
+int test_3 (int data)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_3" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_3," } } */
+  return (obj_3 = data, 0);
+}
+
+extern int volatile obj_4;
+int test_4 (void)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_4" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_4," } } */
+  return (obj_4 = 0, 0);
+}
+extern int volatile obj_5;
+int test_5 (void)
+{
+  /* should reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_5" } } */
+  /* { dg-final { scan-assembler "movl\[ \t\]obj_5," } } */
+  return (obj_5 = 0, obj_5);
+}
+
+/* Assignments in conditional exprs */
+
+extern int volatile obj_6;
+void test_6 (int data, int cond)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_6" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_6," } } */
+  cond ? obj_6 = data : 0;
+}
+
+extern int volatile obj_7;
+int test_7 (int data, int cond)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_7" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_7," } } */
+  return cond ? obj_7 = data : 0;
+}
+
+extern int volatile obj_8;
+int test_8 (int cond)
+{
+  /* should not reread obj */
+  /* { dg-final { scan-assembler "movl\[ \t\]\[^,\]+, obj_8" } } */
+  /* { dg-final { scan-assembler-not "movl\[ \t\]obj_8," } } */
+  return cond ? obj_8 = 0 : 0;
+}

^ permalink raw reply	[flat|nested] 81+ messages in thread

end of thread, other threads:[~2010-08-20 18:11 UTC | newest]

Thread overview: 81+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-21 12:44 [gimple] assignments to volatile Nathan Sidwell
2010-06-21 13:26 ` Richard Guenther
2010-06-21 13:55   ` Michael Matz
2010-06-21 14:07     ` Richard Guenther
2010-06-21 14:09     ` Nathan Sidwell
2010-06-21 14:17       ` Michael Matz
2010-06-21 14:19         ` Richard Guenther
2010-06-21 14:53           ` Michael Matz
2010-06-21 14:17     ` IainS
2010-06-21 14:24       ` Michael Matz
2010-06-21 14:50         ` Nathan Sidwell
2010-06-21 15:24           ` Michael Matz
2010-06-22 11:36             ` Dave Korn
2010-06-23 20:16             ` Mike Stump
2010-06-24 11:51               ` Michael Matz
2010-06-21 15:24         ` Nathan Sidwell
2010-06-21 15:46           ` Michael Matz
2010-06-22 15:37     ` Mark Mitchell
2010-06-22 15:37       ` Jakub Jelinek
2010-06-22 15:57         ` Paul Koning
2010-06-22 15:47       ` Michael Matz
2010-06-22 15:58         ` Mark Mitchell
2010-06-23 11:38           ` Nathan Sidwell
2010-06-23 14:05             ` Mark Mitchell
2010-06-23 14:06               ` Michael Matz
2010-06-23 16:00                 ` Richard Guenther
2010-06-23 16:25               ` Paul Koning
2010-06-23 17:13                 ` Mark Mitchell
2010-06-23 19:16               ` Mike Stump
2010-06-24 10:22                 ` Mark Mitchell
2010-06-24 15:53                   ` Mike Stump
2010-06-24 16:00                     ` Mark Mitchell
2010-06-24 19:37                       ` Mike Stump
2010-06-25  9:37                         ` Nathan Sidwell
2010-06-25 19:06                           ` Mike Stump
2010-06-25 21:33                             ` Mark Mitchell
2010-06-26  9:35                               ` Mike Stump
2010-06-26 10:18                                 ` Mark Mitchell
2010-06-26 12:18                                   ` Richard Guenther
2010-06-26 19:52                                     ` Michael Matz
2010-06-26 19:57                                       ` Mark Mitchell
2010-06-26 20:08                                         ` Michael Matz
2010-06-26 22:13                                           ` Mark Mitchell
2010-06-28  9:20                                             ` Nathan Sidwell
2010-06-28  9:27                                               ` Nathan Sidwell
2010-06-26 10:20                                 ` Richard Kenner
2010-06-28  9:52                             ` Nathan Sidwell
2010-06-30 22:52                               ` Mike Stump
2010-07-05  8:59                                 ` Nathan Sidwell
2010-07-09  5:27                                   ` Mike Stump
2010-07-09  7:22                                     ` Nathan Sidwell
2010-07-16  8:10                                       ` Nathan Sidwell
2010-07-16 15:20                                         ` Mark Mitchell
2010-07-19  8:41                                           ` Nathan Sidwell
2010-08-13  9:56                                           ` Nathan Sidwell
2010-08-18 15:32                                             ` Mark Mitchell
2010-08-18 16:18                                               ` Richard Guenther
2010-08-18 18:04                                                 ` Mike Stump
2010-08-19 11:11                                               ` Nathan Sidwell
2010-08-20  4:22                                                 ` Mark Mitchell
2010-08-20 16:59                                                   ` Mike Stump
2010-08-20 18:00                                             ` H.J. Lu
2010-08-20 18:33                                               ` Nathan Sidwell
2010-06-25  9:20                       ` Nathan Sidwell
2010-06-24 10:23                 ` Nathan Sidwell
2010-06-24 17:05                   ` Mike Stump
2010-06-24 17:29                     ` Paul Koning
2010-06-25  9:26                     ` Nathan Sidwell
2010-06-25 18:20                       ` Mike Stump
2010-06-28  8:49                         ` Nathan Sidwell
2010-07-01  1:02                           ` Mike Stump
2010-07-05  9:02                             ` Nathan Sidwell
2010-07-09  5:14                               ` Mike Stump
2010-07-09  7:20                                 ` Nathan Sidwell
2010-06-22 15:56       ` Paul Koning
2010-06-22 12:08   ` Nathan Sidwell
2010-06-22 12:25     ` Richard Guenther
2010-06-22 13:12     ` Michael Matz
2010-06-22 13:54       ` Nathan Sidwell
2010-06-22 15:21         ` Michael Matz
2010-06-22 16:12           ` Joseph S. Myers

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).