Patchwork [PR,56419/c++] : C++ front end silently drops transactions

login
register
mail settings
Submitter Aldy Hernandez
Date Feb. 22, 2013, 2:53 p.m.
Message ID <5127867A.1000507@redhat.com>
Download mbox | patch
Permalink /patch/222540/
State New
Headers show

Comments

Aldy Hernandez - Feb. 22, 2013, 2:53 p.m.
In the following snippet, the C++ front-end drops the transaction 
altogether:

+int x = 0;
+int inc_func(int i) {
+     for (int j = 0; j < i; ++j)
+     {
+         __transaction_atomic { x+=1; }
+     }
+     return 0;

This was caused by http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=186546

The problem here is that genericize_cp_loop() calls 
append_to_statement_list() to add the TRANSACTION_EXPR, but in this 
case, TREE_SIDE_EFFECTS is not set, so it is silently ignored.

Frankly, I don't understand finish_transaction_stmt(), and why it sets 
TREE_SIDE_EFFECTS only if the [noexcept] clause is set.  I'm C++ 
ignorant, but I would've thought the opposite to be true.

Anyways, I've fixed the problem by setting TREE_SIDE_EFFECTS if the 
transaction body has side effects.  Perhaps we should do this for 
build_transaction_expr() as well?

What do y'all prefer?
commit 01704e6a117846458dbc11cb76284504673f2d3c
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri Feb 22 08:28:14 2013 -0600

    	PR c++/56419
    	* semantics.c (finish_transaction_stmt): Set TREE_SIDE_EFFECTS if
    	the body has side effects.
Jason Merrill - Feb. 25, 2013, 7:43 p.m.
On 02/22/2013 09:53 AM, Aldy Hernandez wrote:
> Frankly, I don't understand finish_transaction_stmt(), and why it sets
> TREE_SIDE_EFFECTS only if the [noexcept] clause is set.  I'm C++
> ignorant, but I would've thought the opposite to be true.

It's setting that flag on the MUST_NOT_THROW_EXPR.

> Anyways, I've fixed the problem by setting TREE_SIDE_EFFECTS if the
> transaction body has side effects.  Perhaps we should do this for
> build_transaction_expr() as well?

I would think managing the transaction boundaries would always have 
side-effects, no?  If that's correct, I'd set the flag unconditionally 
in begin_transaction_stmt and build_transaction_expr.

Jason

Patch

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 458ed26..c446cd6 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -5135,6 +5135,9 @@  finish_transaction_stmt (tree stmt, tree compound_stmt, int flags, tree noex)
       TRANSACTION_EXPR_BODY (stmt) = body;
     }
 
+  if (TREE_SIDE_EFFECTS (TRANSACTION_EXPR_BODY (stmt)))
+    TREE_SIDE_EFFECTS (stmt) = 1;
+
   if (compound_stmt)
     finish_compound_stmt (compound_stmt);
   finish_stmt ();
diff --git a/gcc/testsuite/g++.dg/tm/pr56419.C b/gcc/testsuite/g++.dg/tm/pr56419.C
new file mode 100644
index 0000000..c9a33a8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tm/pr56419.C
@@ -0,0 +1,13 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm" }
+
+int x = 0;
+int inc_func(int i) {
+     for (int j = 0; j < i; ++j)
+     {
+         __transaction_atomic { x+=1; }
+     }
+     return 0;
+}
+
+// { dg-final { scan-assembler "ITM_commitTransaction" } }