This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [trans-mem] fix buglet in expand_assign_tm
- From: Aldy Hernandez <aldyh at redhat dot com>
- To: rth at redhat dot com, gcc-patches at gcc dot gnu dot org
- Date: Tue, 24 Nov 2009 08:30:41 -0400
- Subject: Re: [trans-mem] fix buglet in expand_assign_tm
- References: <20091119182620.GA5011@redhat.com>
Well, Richard okayed the patch but didn't like it, something about a
pass ordering problem which I was sure to be on the hook to fix later.
So might as well fix it correctly now...
The problem here is that if we instrument a store whose LHS is thread
private memory, but whose RHS requires expansion (to an instrumented
variant), we will mark the LHS for logging using the original statement,
but this statement will be removed/replaced during the RHS expansion.
Instead of inserting the unpopular nops in my previous patch, the
following patch calls requires_barrier twice, once without making any
changes to the log, and once if/after we have expanded the assignment,
thus picking up the correct statement (which won't disappear ;-)).
Simpler, cleaner.
OK for branch?
* trans-mem.c (requires_barrier): Document STMT argument.
(expand_assign_tm): Call requires_barrier after assignment has
been expanded.
Index: testsuite/gcc.dg/tm/memopt-6.c
===================================================================
--- testsuite/gcc.dg/tm/memopt-6.c (revision 0)
+++ testsuite/gcc.dg/tm/memopt-6.c (revision 0)
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -O -fdump-tree-tmedge --param tm-max-aggregate-size=1" } */
+
+struct large { int x[100]; };
+struct large bark();
+extern int test (void) __attribute__((transaction_safe));
+struct large lacopy;
+
+int f() /* { dg-message "unimplemented: transactional load" } */
+{
+ int i = readint();
+ struct large lala = bark();
+ __transaction {
+ lala.x[55] = 666;
+ lala = lacopy;
+ }
+ return lala.x[i];
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c (revision 154493)
+++ trans-mem.c (working copy)
@@ -1134,7 +1134,11 @@ static tree lower_sequence_no_tm (gimple
or write barrier.
ENTRY_BLOCK is the entry block for the region where stmt resides
- in. NULL if unknown. */
+ in. NULL if unknown.
+
+ STMT is the statement in which X occurs in. It is used for thread
+ private memory instrumentation. If no TPM instrumentation is
+ desired, STMT should be null. */
static bool
requires_barrier (basic_block entry_block, tree x, gimple stmt)
{
@@ -1739,12 +1743,14 @@ expand_assign_tm (struct tm_region *regi
gimple stmt = gsi_stmt (*gsi);
tree lhs = gimple_assign_lhs (stmt);
tree rhs = gimple_assign_rhs1 (stmt);
- bool store_p = requires_barrier (region->entry_block, lhs, stmt);
+ bool store_p = requires_barrier (region->entry_block, lhs, NULL);
bool load_p = requires_barrier (region->entry_block, rhs, NULL);
gimple gcall;
if (!load_p && !store_p)
{
+ /* Add thread private addresses to log if applicable. */
+ requires_barrier (region->entry_block, lhs, stmt);
gsi_next (gsi);
return;
}
@@ -1774,6 +1780,11 @@ expand_assign_tm (struct tm_region *regi
gcall = build_tm_store (lhs, rhs, gsi);
}
+ /* Now that we have the load/store in its instrumented form, add
+ thread private addresses to the log if applicable. */
+ if (!store_p)
+ requires_barrier (region->entry_block, lhs, gcall);
+
/* add_stmt_to_tm_region (region, gcall); */
}