From patchwork Tue Jun 22 11:16:32 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathan Sidwell X-Patchwork-Id: 56457 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 4BD35B6EF0 for ; Tue, 22 Jun 2010 21:17:41 +1000 (EST) Received: (qmail 6833 invoked by alias); 22 Jun 2010 11:17:37 -0000 Received: (qmail 6813 invoked by uid 22791); 22 Jun 2010 11:17:34 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL, BAYES_00, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 22 Jun 2010 11:17:27 +0000 Received: (qmail 4781 invoked from network); 22 Jun 2010 11:17:25 -0000 Received: from unknown (HELO ?192.168.44.101?) (nathan@127.0.0.2) by mail.codesourcery.com with ESMTPA; 22 Jun 2010 11:17:25 -0000 Message-ID: <4C209B90.6090703@codesourcery.com> Date: Tue, 22 Jun 2010 12:16:32 +0100 From: Nathan Sidwell User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100423 Thunderbird/3.0.4 MIME-Version: 1.0 To: Richard Guenther CC: GCC Patches , Michael Matz Subject: Re: [gimple] assignments to volatile References: <4C1F5380.1090107@codesourcery.com> In-Reply-To: Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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 2010-06-21 Nathan Sidwell Richard Guenther 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; +}