Patchwork PR/51443: fix inline assembly in transaction_callable functions

login
register
mail settings
Submitter Aldy Hernandez
Date Dec. 13, 2011, 3:37 p.m.
Message ID <4EE7713A.5070104@redhat.com>
Download mbox | patch
Permalink /patch/131118/
State New
Headers show

Comments

Aldy Hernandez - Dec. 13, 2011, 3:37 p.m.
A while back we redesigned how the may go irrevocable bit was set.  In 
particular, we redesigned it so that the analysis was done after 
IPA-inline was done, so we didn't make incorrect assumptions about 
irrevocability.

This means that any set or use of the tm_may_enter_irr bit before we 
calculate it in the IPA-tm pass is incorrect.  The following patch 
removes such references.

As is customary, this unearthed a few other buglets and cleanups, which 
I have also addressed.  First, saw_unsafe is no longer needed.  I 
removed it.  Second, ipa_tm_scan_irr_function is now called with builtin 
operators such as new, so we need to exit gracefully when no cfun or CFG 
exists.  Lastly, since we are being slightly more aggressive in 
determining irrevocability, we need to make sure that user annotated 
safe functions are respected.

This fixes the PR.

OK?
PR/51443
	* trans-mem.c (struct diagnose_tm): Remove saw_unsafe.
	(diagnose_tm_1): Same.
	(ipa_tm_execute): Do not test tm_may_enter_irr before we set it.
	(ipa_tm_scan_irr_function): Return gracefully when no
	DECL_STRUCT_FUNCTION.
	(ipa_tm_scan_irr_block): Believe the user on TM attributes.
Richard Henderson - Dec. 13, 2011, 5:10 p.m.
On 12/13/2011 07:37 AM, Aldy Hernandez wrote:
> 	PR/51443
> 	* trans-mem.c (struct diagnose_tm): Remove saw_unsafe.
> 	(diagnose_tm_1): Same.
> 	(ipa_tm_execute): Do not test tm_may_enter_irr before we set it.
> 	(ipa_tm_scan_irr_function): Return gracefully when no
> 	DECL_STRUCT_FUNCTION.
> 	(ipa_tm_scan_irr_block): Believe the user on TM attributes.

Ok.


r~
Eric Botcazou - Dec. 13, 2011, 5:49 p.m.
> This fixes the PR.

You need to put the Component before the / in the ChangeLog, otherwise the 
commit won't be cross-referenced in Bugzilla.
Aldy Hernandez - Dec. 13, 2011, 5:52 p.m.
On 12/13/11 11:49, Eric Botcazou wrote:
>> This fixes the PR.
>
> You need to put the Component before the / in the ChangeLog, otherwise the
> commit won't be cross-referenced in Bugzilla.
>

Funny, I just noticed.  I will do so from now on.

Thanks.

Patch

Index: testsuite/gcc.dg/tm/asm-1.c
===================================================================
--- testsuite/gcc.dg/tm/asm-1.c	(revision 0)
+++ testsuite/gcc.dg/tm/asm-1.c	(revision 0)
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -O1" } */
+
+static inline void asmfunc()
+{
+__asm__("");
+}
+
+__attribute__((transaction_callable))
+void push()
+{
+        asmfunc();
+}
Index: testsuite/g++.dg/tm/asm-1.c
===================================================================
--- testsuite/g++.dg/tm/asm-1.c	(revision 0)
+++ testsuite/g++.dg/tm/asm-1.c	(revision 0)
@@ -0,0 +1,22 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm -O1" }
+
+template<class T> class shared_ptr {
+public:
+    shared_ptr()  {
+      __asm__ ("");
+    }
+};
+template<typename _Tp> class deque {
+public:
+    void push_back() {
+      ::new _Tp();
+    }
+};
+class Bar {
+  __attribute__((transaction_callable)) void push();
+  deque<shared_ptr<int> > events;
+};
+void Bar::push() {
+  events.push_back();
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 182244)
+++ trans-mem.c	(working copy)
@@ -544,7 +544,6 @@  struct diagnose_tm
   unsigned int summary_flags : 8;
   unsigned int block_flags : 8;
   unsigned int func_flags : 8;
-  unsigned int saw_unsafe : 1;
   unsigned int saw_volatile : 1;
   gimple stmt;
 };
@@ -695,8 +694,6 @@  diagnose_tm_1 (gimple_stmt_iterator *gsi
       else if (d->func_flags & DIAG_TM_SAFE)
 	error_at (gimple_location (stmt),
 		  "asm not allowed in %<transaction_safe%> function");
-      else
-	d->saw_unsafe = true;
       break;
 
     case GIMPLE_TRANSACTION:
@@ -711,8 +708,6 @@  diagnose_tm_1 (gimple_stmt_iterator *gsi
 	    else if (d->func_flags & DIAG_TM_SAFE)
 	      error_at (gimple_location (stmt),
 			"relaxed transaction in %<transaction_safe%> function");
-	    else
-	      d->saw_unsafe = true;
 	    inner_flags = DIAG_TM_RELAXED;
 	  }
 	else if (gimple_transaction_subcode (stmt) & GTMA_IS_OUTER)
@@ -727,8 +722,6 @@  diagnose_tm_1 (gimple_stmt_iterator *gsi
 	    else if (d->func_flags & DIAG_TM_SAFE)
 	      error_at (gimple_location (stmt),
 			"outer transaction in %<transaction_safe%> function");
-	    else
-	      d->saw_unsafe = true;
 	    inner_flags |= DIAG_TM_OUTER;
 	  }
 
@@ -748,8 +741,6 @@  diagnose_tm_1 (gimple_stmt_iterator *gsi
 
 	    walk_gimple_seq (gimple_transaction_body (stmt),
 			     diagnose_tm_1, diagnose_tm_1_op, &wi_inner);
-
-	    d->saw_unsafe |= d_inner.saw_unsafe;
 	  }
       }
       break;
@@ -780,11 +771,6 @@  diagnose_tm_blocks (void)
   walk_gimple_seq (gimple_body (current_function_decl),
 		   diagnose_tm_1, diagnose_tm_1_op, &wi);
 
-  /* If we saw something other than a call that makes this function
-     unsafe, remember it so that the IPA pass only needs to scan calls.  */
-  if (d.saw_unsafe && !is_tm_safe_or_pure (current_function_decl))
-    cgraph_local_info (current_function_decl)->tm_may_enter_irr = 1;
-
   return 0;
 }
 
@@ -3696,7 +3682,11 @@  ipa_tm_scan_irr_block (basic_block bb)
 		break;
 
 	      d = get_cg_data (cgraph_get_node (fn));
-	      if (d->is_irrevocable)
+
+	      /* Return true if irrevocable, but above all, believe
+		 the user.  */
+	      if (d->is_irrevocable
+		  && !is_tm_safe_or_pure (fn))
 		return true;
 	    }
 	  break;
@@ -3880,6 +3870,11 @@  ipa_tm_scan_irr_function (struct cgraph_
   VEC (basic_block, heap) *queue;
   bool ret = false;
 
+  /* Builtin operators (operator new, and such).  */
+  if (DECL_STRUCT_FUNCTION (node->decl) == NULL
+      || DECL_STRUCT_FUNCTION (node->decl)->cfg == NULL)
+    return false;
+
   current_function_decl = node->decl;
   push_cfun (DECL_STRUCT_FUNCTION (node->decl));
   calculate_dominance_info (CDI_DOMINATORS);
@@ -4689,14 +4684,11 @@  ipa_tm_execute (void)
 	    /* Scan for calls that are in each transaction.  */
 	    ipa_tm_scan_calls_transaction (d, &tm_callees);
 
-	    /* If we saw something that will make us go irrevocable, put it
-	       in the worklist so we can scan the function later
-	       (ipa_tm_scan_irr_function) and mark the irrevocable blocks.  */
-	    if (node->local.tm_may_enter_irr)
-	      {
-		maybe_push_queue (node, &irr_worklist, &d->in_worklist);
-		d->want_irr_scan_normal = true;
-	      }
+	    /* Put it in the worklist so we can scan the function
+	       later (ipa_tm_scan_irr_function) and mark the
+	       irrevocable blocks.  */
+	    maybe_push_queue (node, &irr_worklist, &d->in_worklist);
+	    d->want_irr_scan_normal = true;
 	  }
 
 	pop_cfun ();
@@ -4712,11 +4704,10 @@  ipa_tm_execute (void)
       a = cgraph_function_body_availability (node);
       d = get_cg_data (node);
 
-      /* If we saw something that will make us go irrevocable, put it
-	 in the worklist so we can scan the function later
-	 (ipa_tm_scan_irr_function) and mark the irrevocable blocks.  */
-      if (node->local.tm_may_enter_irr)
-	maybe_push_queue (node, &irr_worklist, &d->in_worklist);
+      /* Put it in the worklist so we can scan the function later
+	 (ipa_tm_scan_irr_function) and mark the irrevocable
+	 blocks.  */
+      maybe_push_queue (node, &irr_worklist, &d->in_worklist);
 
       /* Some callees cannot be arbitrarily cloned.  These will always be
 	 irrevocable.  Mark these now, so that we need not scan them.  */