diff mbox

[trans-mem] Fix instantiation of transaction expressions.

Message ID 1320694411.32515.6.camel@triegel.csb
State New
Headers show

Commit Message

Torvald Riegel Nov. 7, 2011, 7:33 p.m. UTC
On Mon, 2011-11-07 at 14:04 -0500, Jason Merrill wrote:
> 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

I didn't claim it was pretty ... :)

>  but ought to work fine, and 
> there doesn't seem to be a simple test to distinguish between statements 
> and expressions currently.

As suggested by Richard, here's an updated patch that uses
TREE_LANG_FLAG_0 to keep track of whether a TRANSACTION_EXPR contains
statements or an expression. I also split out the code to create a
transaction expression.

TM tests seem to run fine. OK for branch pending bootstrap?
commit 6f0835817e263d2b28d609096305f94459799af2
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.
    	* cp/cp-tree.h (TRANSACTION_EXPR_IS_STMT, build_transaction_expr): New.
    	* cp/parser.c (cp_parser_transaction_expression): Use
    	build_transaction_expr.
    	* cp/semantics.c (build_transaction_expr): New.
    	(finish_transaction_stmt): Set TRANSACTION_EXPR_IS_STMT.
    	* testsuite/g++.dg/tm/template-1.C: New.

Comments

Richard Henderson Nov. 7, 2011, 7:46 p.m. UTC | #1
On 11/07/2011 11:33 AM, Torvald Riegel wrote:
>     Fix instantiation of transaction expressions.
>     
>     	* cp/pt.c (tsubst_expr) [TRANSACTION_EXPR]: If body is not a
>     	statement, create an expression instead.
>     	* cp/cp-tree.h (TRANSACTION_EXPR_IS_STMT, build_transaction_expr): New.
>     	* cp/parser.c (cp_parser_transaction_expression): Use
>     	build_transaction_expr.
>     	* cp/semantics.c (build_transaction_expr): New.
>     	(finish_transaction_stmt): Set TRANSACTION_EXPR_IS_STMT.
>     	* testsuite/g++.dg/tm/template-1.C: New.

Looks good to me.

> +            tmp = RECUR (TRANSACTION_EXPR_BODY (t));

Dead store now.


r~
diff mbox

Patch

--- a/gcc/cp/ChangeLog.tm-merge
+++ b/gcc/cp/ChangeLog.tm-merge
@@ -10,7 +10,9 @@ 
 	(look_for_tm_attr_overrides, set_one_vmethod_tm_attributes,
 	set_method_tm_attributes): New.
 	(finish_struct_1): Call set_method_tm_attributes.
-	* cp-tree.h (begin_transaction_stmt, finish_transaction_stmt): Declare.
+	* cp-tree.h (begin_transaction_stmt, finish_transaction_stmt,
+	build_transaction_expr): Declare.
+	(TRANSACTION_EXPR_IS_STMT): New.
 	* decl.c (push_cp_library_fn): Set attribute to transaction_safe.
 	* except.c (do_get_exception_ptr): Apply transaction_pure.
 	(do_begin_catch): Mark _ITM_cxa_begin_catch transaction_pure and
@@ -30,4 +32,5 @@ 
 	cp_parser_token_starts_function_definition_p): Same.
 	(cp_parser_required_error): Handle RT_TRANSACTION*.
 	* pt.c (tsubst_expr): Handle TRANSACTION_EXPR.
-	* semantics.c (begin_transaction_stmt, finish_transaction_stmt): New.
\ No newline at end of file
+	* semantics.c (begin_transaction_stmt, finish_transaction_stmt,
+	build_transaction_expr): New.
\ No newline at end of file
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index b6468e6..1bb6d98 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -73,6 +73,7 @@  c-common.h, not after.
       VEC_INIT_EXPR_IS_CONSTEXPR (in VEC_INIT_EXPR)
       DECL_OVERRIDE_P (in FUNCTION_DECL)
       IMPLICIT_CONV_EXPR_DIRECT_INIT (in IMPLICIT_CONV_EXPR)
+      TRANSACTION_EXPR_IS_STMT (in TRANSACTION_EXPR)
    1: IDENTIFIER_VIRTUAL_P (in IDENTIFIER_NODE)
       TI_PENDING_TEMPLATE_FLAG.
       TEMPLATE_PARMS_FOR_INLINE.
@@ -3862,6 +3863,10 @@  more_aggr_init_expr_args_p (const aggr_init_expr_arg_iterator *iter)
   TREE_TYPE (OMP_CLAUSE_RANGE_CHECK (NODE, OMP_CLAUSE_PRIVATE, \
 				     OMP_CLAUSE_COPYPRIVATE))
 
+/* Nonzero if this transaction expression's body contains statements.  */
+#define TRANSACTION_EXPR_IS_STMT(NODE) \
+   TREE_LANG_FLAG_0 (TRANSACTION_EXPR_CHECK (NODE))
+
 /* These macros provide convenient access to the various _STMT nodes
    created when parsing template declarations.  */
 #define TRY_STMTS(NODE)		TREE_OPERAND (TRY_BLOCK_CHECK (NODE), 0)
@@ -5527,6 +5532,7 @@  extern void finish_omp_flush			(void);
 extern void finish_omp_taskwait			(void);
 extern tree begin_transaction_stmt		(location_t, tree *, int);
 extern void finish_transaction_stmt		(tree, tree, int);
+extern tree build_transaction_expr		(location_t, tree, int);
 extern void finish_omp_taskyield		(void);
 extern bool cxx_omp_create_clause_info		(tree, tree, bool, bool, bool);
 extern tree baselink_for_fns                    (tree);
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index b6ac918..334ed22 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -26682,10 +26682,7 @@  cp_parser_transaction_expression (cp_parser *parser, enum rid keyword)
   if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_PAREN))
     {
       tree expr = cp_parser_expression (parser, /*cast_p=*/false, NULL);
-      ret = build1 (TRANSACTION_EXPR, TREE_TYPE (expr), expr);
-      if (this_in & TM_STMT_ATTR_RELAXED)
-	TRANSACTION_EXPR_RELAXED (ret) = 1;
-      SET_EXPR_LOCATION (ret, token->location);
+      ret = build_transaction_expr (token->location, expr, this_in);
     }
   else
     {
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index b1593fe..d4df8de 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -12958,9 +12958,19 @@  tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
         flags |= (TRANSACTION_EXPR_OUTER (t) ? TM_STMT_ATTR_OUTER : 0);
         flags |= (TRANSACTION_EXPR_RELAXED (t) ? TM_STMT_ATTR_RELAXED : 0);
 
-        stmt = begin_transaction_stmt (input_location, NULL, flags);
-        tmp = RECUR (TRANSACTION_EXPR_BODY (t));
-        finish_transaction_stmt (stmt, NULL, flags);
+        if (TRANSACTION_EXPR_IS_STMT (t))
+          {
+            stmt = begin_transaction_stmt (input_location, NULL, flags);
+            tmp = RECUR (TRANSACTION_EXPR_BODY (t));
+            finish_transaction_stmt (stmt, NULL, flags);
+          }
+        else
+          {
+            stmt = build_transaction_expr (EXPR_LOCATION (t),
+					   RECUR (TRANSACTION_EXPR_BODY (t)),
+					   flags);
+            return stmt;
+          }
       }
       break;
 
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index d1967ee..068754a 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -5003,11 +5003,25 @@  finish_transaction_stmt (tree stmt, tree compound_stmt, int flags)
   TRANSACTION_EXPR_BODY (stmt) = pop_stmt_list (TRANSACTION_EXPR_BODY (stmt));
   TRANSACTION_EXPR_OUTER (stmt) = (flags & TM_STMT_ATTR_OUTER) != 0;
   TRANSACTION_EXPR_RELAXED (stmt) = (flags & TM_STMT_ATTR_RELAXED) != 0;
+  TRANSACTION_EXPR_IS_STMT (stmt) = 1;
 
   if (compound_stmt)
     finish_compound_stmt (compound_stmt);
   finish_stmt ();
 }
+
+/* Build a __transaction_atomic or __transaction_relaxed expression.  */
+
+tree
+build_transaction_expr (location_t loc, tree expr, int flags)
+{
+  tree ret;
+  ret = build1 (TRANSACTION_EXPR, TREE_TYPE (expr), expr);
+  if (flags & TM_STMT_ATTR_RELAXED)
+	TRANSACTION_EXPR_RELAXED (ret) = 1;
+  SET_EXPR_LOCATION (ret, loc);
+  return ret;
+}
 
 void
 init_cp_semantics (void)
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" } } */