diff mbox

[trans-mem] do not go through the run-time for clone calls

Message ID 4D5C243D.2070701@redhat.com
State New
Headers show

Commit Message

Richard Henderson Feb. 16, 2011, 7:23 p.m. UTC 
On 02/16/2011 08:59 AM, Aldy Hernandez wrote:
> And now for the actual optimization...
> 
> There is no need to go through the run-time when calling a clone.  We can just call the clone directly.
> 
> OK?

I think this is a lot closer to correct.  In the process I found some bits
that look... odd to say the least.


r~

Comments

Torvald Riegel Feb. 17, 2011, 10:05 a.m. UTC | #1 
On Wednesday 16 February 2011 20:23:41 Richard Henderson wrote:
> I think this is a lot closer to correct.  In the process I found some bits
> that look... odd to say the least.

From the patch:
> Also note that find_tm_replacement_function also
> +	 contains mappings into the TM runtime, e.g. memcpy.  These
> +	 we know won't go irrevocable.

I think TM runtime functions can always go irrevocable (most TM libs assume 
this I think). Nevertheless, this should not affect the bits (hasNoIrrecovable 
and doesGoIrrevocable) passed into beginTransaction().

The ABI explicitely associates the hasNoIrrevocable bit with calls to 
changeTransactionMode(SERIAL), which we might want to extend to 
getTMCloneOrIrrevocable(). However, the purpose of this bit isn't clear to me, 
and IMO it needs clarification in the ABI.

Torvald
Richard Henderson Feb. 17, 2011, 4:51 p.m. UTC | #2 
On 02/17/2011 02:05 AM, Torvald Riegel wrote:
> On Wednesday 16 February 2011 20:23:41 Richard Henderson wrote:
>> I think this is a lot closer to correct.  In the process I found some bits
>> that look... odd to say the least.
> 
> From the patch:
>> Also note that find_tm_replacement_function also
>> +	 contains mappings into the TM runtime, e.g. memcpy.  These
>> +	 we know won't go irrevocable.
> 
> I think TM runtime functions can always go irrevocable (most TM libs assume 
> this I think). Nevertheless, this should not affect the bits (hasNoIrrecovable 
> and doesGoIrrevocable) passed into beginTransaction().

Agreed that the TM can do whatever it needs under the covers.  The point I
was making was all about getting the hasNoIrrevocable bit set properly.


r~
diff mbox

Patch

diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c
index a7190d5..e15605d 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -4294,7 +4294,28 @@  ipa_tm_transform_calls_redirect (struct cgraph_node *node,
   struct cgraph_edge *e = cgraph_edge (node, stmt);
   tree fndecl = gimple_call_fndecl (stmt);
 
-  /* If there is a replacement, use it, otherwise use the clone.  */
+  /* For indirect calls, pass the address through the runtime.  */
+  if (fndecl == NULL)
+    {
+      *need_ssa_rename_p |=
+	ipa_tm_insert_gettmclone_call (node, region, gsi, stmt);
+      return;
+    }
+
+  /* If the call is to the TM runtime, do nothing further.  */
+  if (flags_from_decl_or_type (fndecl) & ECF_TM_OPS)
+    return;
+
+  /* Fixup recursive calls inside clones.  */
+  /* ??? Why did cgraph_copy_node_for_versioning update the call edges 
+     for recursion but not update the call statements themselves?  */
+  if (e->caller == e->callee && DECL_IS_TM_CLONE (current_function_decl))
+    {
+      gimple_call_set_fndecl (stmt, current_function_decl);
+      return;
+    }
+
+  /* If there is a replacement, use it.  */
   fndecl = find_tm_replacement_function (fndecl);
   if (fndecl)
     {
@@ -4313,6 +4334,11 @@  ipa_tm_transform_calls_redirect (struct cgraph_node *node,
 
 	 We do need to mark these nodes so that we get the proper
 	 result in expand_call_tm.  */
+      /* ??? This seems broken.  How is it that we're marking the
+	 CALLEE as may_enter_irr?  Surely we should be marking the
+	 CALLER.  Also note that find_tm_replacement_function also
+	 contains mappings into the TM runtime, e.g. memcpy.  These
+	 we know won't go irrevocable.  */
       new_node->local.tm_may_enter_irr = 1;
     }
   else
@@ -4358,27 +4384,12 @@  ipa_tm_transform_calls_1 (struct cgraph_node *node, struct tm_region *region,
   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
     {
       gimple stmt = gsi_stmt (gsi);
-      tree fndecl;
 
       if (!is_gimple_call (stmt))
 	continue;
       if (is_tm_pure_call (stmt))
 	continue;
 
-      fndecl = gimple_call_fndecl (stmt);
-
-      /* For indirect calls, pass the address through the runtime.  */
-      if (fndecl == NULL)
-	{
-	  need_ssa_rename |=
-	    ipa_tm_insert_gettmclone_call (node, region, &gsi, stmt);
-	  continue;
-	}
-
-      /* Don't scan past the end of the transaction.  */
-      if (is_tm_ending_fndecl (fndecl))
-	break;
-
       /* Redirect edges to the appropriate replacement or clone.  */
       ipa_tm_transform_calls_redirect (node, region, &gsi, &need_ssa_rename);
     }