From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28834 invoked by alias); 6 Jan 2007 13:19:57 -0000 Received: (qmail 28823 invoked by uid 22791); 6 Jan 2007 13:19:55 -0000 X-Spam-Check-By: sourceware.org Received: from province.act-europe.fr (HELO province.act-europe.fr) (212.157.227.214) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 06 Jan 2007 13:19:50 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-province.act-europe.fr (Postfix) with ESMTP id 824D24AD57 for ; Sat, 6 Jan 2007 14:19:47 +0100 (CET) Received: from province.act-europe.fr ([127.0.0.1]) by localhost (province.act-europe.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 54565-09 for ; Sat, 6 Jan 2007 14:19:47 +0100 (CET) Received: from [192.168.1.3] (unknown [91.164.38.58]) (using TLSv1 with cipher RC4-MD5 (128/128 bits)) (No client certificate requested) by province.act-europe.fr (Postfix) with ESMTP id CA3F64AD55 for ; Sat, 6 Jan 2007 14:19:46 +0100 (CET) From: Eric Botcazou To: gcc-patches@gcc.gnu.org Subject: [PATCH] Tree SRA and atomicity/volatility Date: Sat, 06 Jan 2007 13:19:00 -0000 User-Agent: KMail/1.7.1 MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_fK6nFtfQI5eXY/m" Message-Id: <200701061422.39157.ebotcazou@adacore.com> Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2007-01/txt/msg00457.txt.bz2 --Boundary-00=_fK6nFtfQI5eXY/m Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 3538 Hi, The Ada language has the concept of atomicity, for example: type Word is record First, Second, Third, Fourth : Byte; end record; External : Word := (others => 0); pragma Atomic (External); Quoting the Ada RM: "For an atomic object (including an atomic component) all reads and updates of the object as a whole are indivisible." The GNAT compiler implements it by (among other things) declaring the object as being volatile. While this works well in 3.x, this breaks in 4.x because the SRA may decompose accesses to volatile aggregates: eric@linux:~/build/gcc/native> cat t.c struct S { char a; char b; char c; char d; }; void foo(void) { volatile struct S dev; struct S tmp = dev; tmp.a = 0; dev = tmp; } In .sra at -O: ;; Function foo (foo) Cannot scalarize variable dev because it is declared volatile [...] foo () { char tmp$d; char tmp$c; char tmp$b; char tmp$a; struct S tmp; struct S dev; : tmp$d_6 = dev.d; tmp$c_7 = dev.c; tmp$b_8 = dev.b; tmp$a_9 = dev.a; tmp$a_10 = 0; dev.d = tmp$d_6; dev.c = tmp$c_7; dev.b = tmp$b_8; dev.a = tmp$a_10; return; } but accesses to the volatile object are nevertheless decomposed. On x86-64: foo: .LFB2: movzbl -21(%rsp), %eax movzbl -22(%rsp), %edx movzbl -23(%rsp), %ecx movzbl -24(%rsp), %esi movb %al, -21(%rsp) movb %dl, -22(%rsp) movb %cl, -23(%rsp) movb $0, -24(%rsp) ret While this may or may not be a problem for the C language, I think that it's a real bug in the Tree SRA pass, which doesn't check whether the object to be decomposed has side-effects. More specifically, this is the LDST case: /* Invoked when we have a copy between one scalarizable reference ELT and one non-scalarizable reference OTHER. IS_OUTPUT is true if ELT is on the left-hand side. */ void (*ldst) (struct sra_elt *elt, tree other, block_stmt_iterator *bsi, bool is_output); /* Scalarize a LDST. To recap, this is an assignment between one scalarizable reference ELT and one non-scalarizable reference OTHER. IS_OUTPUT is true if ELT is on the left-hand side. */ static void scalarize_ldst (struct sra_elt *elt, tree other, block_stmt_iterator *bsi, bool is_output) { /* Shouldn't have gotten called for a scalar. */ gcc_assert (!elt->replacement); if (elt->use_block_copy) { /* Since ELT is not fully instantiated, we have to leave the block copy in place. Treat this as a USE. */ scalarize_use (elt, NULL, bsi, is_output, false); } else { /* The interesting case is when ELT is fully instantiated. In this case we can have each element stored/loaded directly to/from the corresponding slot in OTHER. This avoids a block copy. */ The last comment is wrong if OTHER has side-effects. Hence the attached patch, tested on x86_64-suse-linux, which yields on x86-64: foo: .LFB2: movl -24(%rsp), %eax movb $0, %al movl %eax, -24(%rsp) ret This is a regression from the 3.x series for the Ada compiler so I'm requesting approval for mainline/4.2/4.1 branches. 2007-01-06 Eric Botcazou * tree-sra.c (sra_walk_fns) : Document new restriction. (sra_walk_modify_expr) : Treat the reference as a use if the lhs has side-effects. : Treat the reference as a use if the rhs has side-effects. :ADDPATCH Tree-SRA: -- Eric Botcazou --Boundary-00=_fK6nFtfQI5eXY/m Content-Type: text/x-diff; charset="us-ascii"; name="fc04-007_fsf.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="fc04-007_fsf.diff" Content-length: 1606 Index: tree-sra.c =================================================================== --- tree-sra.c (revision 120483) +++ tree-sra.c (working copy) @@ -670,8 +670,8 @@ struct sra_walk_fns void (*init) (struct sra_elt *elt, tree value, block_stmt_iterator *bsi); /* Invoked when we have a copy between one scalarizable reference ELT - and one non-scalarizable reference OTHER. IS_OUTPUT is true if ELT - is on the left-hand side. */ + and one non-scalarizable reference OTHER without side-effects. + IS_OUTPUT is true if ELT is on the left-hand side. */ void (*ldst) (struct sra_elt *elt, tree other, block_stmt_iterator *bsi, bool is_output); @@ -887,7 +887,7 @@ sra_walk_gimple_modify_stmt (tree expr, /* If the RHS is scalarizable, handle it. There are only two cases. */ if (rhs_elt) { - if (!rhs_elt->is_scalar) + if (!rhs_elt->is_scalar && !TREE_SIDE_EFFECTS (lhs)) fns->ldst (rhs_elt, lhs, bsi, false); else fns->use (rhs_elt, &GIMPLE_STMT_OPERAND (expr, 1), bsi, false, false); @@ -930,7 +930,8 @@ sra_walk_gimple_modify_stmt (tree expr, The lvalue requirement prevents us from trying to directly scalarize the result of a function call. Which would result in trying to call the function multiple times, and other evil things. */ - else if (!lhs_elt->is_scalar && is_gimple_addressable (rhs)) + else if (!lhs_elt->is_scalar + && !TREE_SIDE_EFFECTS (rhs) && is_gimple_addressable (rhs)) fns->ldst (lhs_elt, rhs, bsi, true); /* Otherwise we're being used in some context that requires the --Boundary-00=_fK6nFtfQI5eXY/m--