This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH] Tree SRA and atomicity/volatility
- From: Eric Botcazou <ebotcazou at adacore dot com>
- To: gcc-patches at gcc dot gnu dot org
- Date: Sat, 6 Jan 2007 14:22:39 +0100
- Subject: [PATCH] Tree SRA and atomicity/volatility
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;
<bb 2>:
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 <ebotcazou@adacore.com>
* tree-sra.c (sra_walk_fns) <ldst>: Document new restriction.
(sra_walk_modify_expr) <rhs_elt>: Treat the reference as a use
if the lhs has side-effects.
<lhs_elt>: Treat the reference as a use if the rhs has side-effects.
:ADDPATCH Tree-SRA:
--
Eric Botcazou
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