Patchwork [trans-mem] Fix instantiation of transaction expressions.

login
register
mail settings
Submitter Torvald Riegel
Date Nov. 7, 2011, 2:12 p.m.
Message ID <1320675134.18023.349.camel@triegel.csb>
Download mbox | patch
Permalink /patch/124080/
State New
Headers show

Comments

Torvald Riegel - Nov. 7, 2011, 2:12 p.m.
I stumbled upon this when working on noexcept. When txn expressions were
processed in tsubst_expr(), the previous code incorrectly created empty
transaction statements because no statements got added to the statement
list used by (begin|finish)_transaction_stmt(). Also,
"return __transaction_atomic (x+1);" incorrectly returned an error that
this wouldn't return a value.
With the patch, if RECUR(...) returns a value, then we assume that this
is an expression and create a txn expression instead.

No regressions for the TM tests, running a bootstrap currently.

OK for branch?
commit 437bd75328aa06561cb84f5e419431f979776115
Author: Torvald Riegel <triegel@redhat.com>
Date:   Mon Nov 7 14:51:50 2011 +0100

    Fix instantiation of transaction expressions.
    
    	* cp/pt.c (tsubst_expr) [TRANSACTION_EXPR]: If body is not a
    	statement, create an expression instead.
    	* testsuite/g++.dg/tm/template-1.C: New.
Richard Henderson - Nov. 7, 2011, 4:31 p.m.
On 11/07/2011 06:12 AM, Torvald Riegel wrote:
>          stmt = begin_transaction_stmt (input_location, NULL, flags);
>          tmp = RECUR (TRANSACTION_EXPR_BODY (t));
> +        if (tmp)
> +          {
> +            /* No statements; handle this like an expression.  */

In which case I'm pretty sure you ought to check for non-null
TRANSACTION_EXPR_BODY first and not call begin_transaction_stmt.


r~
Jason Merrill - Nov. 7, 2011, 7:04 p.m.
On 11/07/2011 11:31 AM, Richard Henderson wrote:
> On 11/07/2011 06:12 AM, Torvald Riegel wrote:
>>           stmt = begin_transaction_stmt (input_location, NULL, flags);
>>           tmp = RECUR (TRANSACTION_EXPR_BODY (t));
>> +        if (tmp)
>> +          {
>> +            /* No statements; handle this like an expression.  */
>
> In which case I'm pretty sure you ought to check for non-null
> TRANSACTION_EXPR_BODY first and not call begin_transaction_stmt.

I believe TRANSACTION_EXPR_BODY is always non-null, but when tsubst_expr 
is called for a statement it adds the new statement via add_stmt and 
returns null.  This approach is a bit funny, but ought to work fine, and 
there doesn't seem to be a simple test to distinguish between statements 
and expressions currently.

Jason

Patch

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index b1593fe..6e6dcdb 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -12960,6 +12960,16 @@  tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
 
         stmt = begin_transaction_stmt (input_location, NULL, flags);
         tmp = RECUR (TRANSACTION_EXPR_BODY (t));
+        if (tmp)
+          {
+            /* No statements; handle this like an expression.  */
+            pop_stmt_list (TRANSACTION_EXPR_BODY (stmt));
+            stmt = build1 (TRANSACTION_EXPR, TREE_TYPE (tmp), tmp);
+            TRANSACTION_EXPR_OUTER (stmt) = TRANSACTION_EXPR_OUTER (t);
+            TRANSACTION_EXPR_RELAXED (stmt) = TRANSACTION_EXPR_RELAXED (t);
+            SET_EXPR_LOCATION (stmt, EXPR_LOCATION (t));
+            return stmt;
+          }
         finish_transaction_stmt (stmt, NULL, flags);
       }
       break;
diff --git a/gcc/testsuite/g++.dg/tm/template-1.C b/gcc/testsuite/g++.dg/tm/template-1.C
new file mode 100644
index 0000000..b93828a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tm/template-1.C
@@ -0,0 +1,35 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm -O -fdump-tree-tmmark" }
+
+struct TrueFalse
+{
+  static bool v() { return true; }
+};
+
+int global;
+
+template<typename T> int foo()
+{
+  __transaction_atomic { global += 2; }
+  return __transaction_atomic (global + 1);
+}
+
+template<typename T> int bar() __transaction_atomic
+{
+  return global + 3;
+}
+
+template<typename T> void bar2() __transaction_atomic
+{
+  global += 4;
+}
+
+int f1()
+{
+  bar2<TrueFalse>();
+  return foo<TrueFalse>() + bar<TrueFalse>();
+}
+
+/* 4 transactions overall, two of the write to global:  */
+/* { dg-final { scan-tree-dump-times "ITM_RU4\\s*\\(&global" 4 "tmmark" } } */
+/* { dg-final { scan-tree-dump-times "ITM_WU4\\s*\\(&global" 2 "tmmark" } } */