This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [gimple] assignments to volatile


On 06/21/10 13:07, Richard Guenther wrote:

Hm, TREE_THIS_VOLATILE (TREE_TYPE (*to_p)) should
certainly be TREE_THIS_VOLATILE (*to_p), and all this
should be done after gimplifying *to_p.  I think even just
before we build the gimple assign/call do
...

Thanks! here's a tested patch that implements it the way you suggest.

Now, as for the semantics we want. I disagree with Michael's opinion.

Firstly, C and C++ differ in their std's description of the semantics of an expression cast to void (implicitly or explicitly). The C std essentially says the value is discarded. The C++ std specifies that no lvalue to rvalue transformation occurs, and it is this transformation that causes an lvalue object to be read. There was a long discussion about this several years ago when I patched G++ to have somewhat more useful semantics (and warnings) than it did have. That series of patches started when I noticed that in C++ '(void)*vol_ptr' did not cause a read of the pointed-to object, but it did in C. The conclusion at that time were that the implemented C semantics were what was wanted. These were:
* after an assignment to a volatile, no re-read occurred.
* a volatile was always read in a void context.


These rules are
* simple
* composable

It means that:

'*vol_ptr1 = *vol_ptr2 = 0'
doesn't read *vol_ptr2 and therefore assign an unknown value to *vol_ptr1. It also doesn't force any ordering on the two assignments to *vol_ptr1 and *vol_ptr2 -- just as would be true if things were not volatile.


'*vol_ptr1'
where this is not the lhs of an assignment, will read the volatile object being pointed to, regardless of whether we use the value or not.


Notice in those examples that I have statement fragments. The semantics are the same regardless of the context in which those fragments occur.

IIUC, Michael's desired semantics are
*) after an assignment, the value is re-read iff the assignment's value is used.
*) a volatile object is not read in a void context.
*) a volatile sub-expression is read or re-read if the value is used by the containing expression [I am unsure if this is Michael's semantics or not, please correct me if I'm wrong]


I am unsure whether the behaviour Michael would like is dependent on optimization level. I think that would be a very bad thing, btw.

I think these lead to confusing code sequences, and are not composable. Fundamentally the ambiguity comes from specifying when an assignment's value is needed. Without even considering data-flow optimizations that could determine a value is unused, we have the following:

'cond ? (*vol_ptr = v) : other_v;'

The conditional expression has a value, even though it is in a void context. Does that mean that the volatile assignment's value is used or not? Similarly for:
'something, (*vol_ptr = v);'
or a more complicated variants:
'something, ((vol_ptr = v), something);'
'(something, (vol_ptr = v)), something;'


the problem here is that the behaviour of a sub-expression now depends on the context in which it is embedded (and that could be quite deeply embedded). If we decide that the above examples do not re-read, because the value is ultimately not used, we have the ability for the programmer to really get confused during debugging. For instance, they may rewrite one of these as
'tmp = cond ? (*vol_ptr = v) : other_v;'
in order to determine the value that was written. But now we've changed the pattern of accesses to the volatile object. (And by in my recent dive into gimplify, we'll have a harder patch to write as we need to track want_result through more structures than we do at the moment.)


Should whatever semantics we decide upon be implemented by the gimplifier or by the front-end? I'm not sure, but I do think the gimplifier should specify the semantics of volatile accesses, and those semantics should be consistent. Currently they are not.

nathan

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

2010-06-21  Nathan Sidwell  <nathan@codesourcery.com>
	    Richard Guenther  <richard.guenther@gmail.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)
@@ -4572,6 +4572,9 @@
       SET_DECL_DEBUG_EXPR (*from_p, *to_p);
    }
 
+  if (want_value && TREE_THIS_VOLATILE (*to_p))
+    *from_p = get_initialized_tmp_var (*from_p, pre_p, post_p);
+
   if (TREE_CODE (*from_p) == CALL_EXPR)
     {
       /* Since the RHS is a CALL_EXPR, we need to create a GIMPLE_CALL
@@ -4599,7 +4602,7 @@
 
   if (want_value)
     {
-      *expr_p = unshare_expr (*to_p);
+      *expr_p = TREE_THIS_VOLATILE (*to_p) ? *from_p : unshare_expr (*to_p);
       return GS_OK;
     }
   else
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;
+}

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]