Patchwork [trans-mem] PR47690: redirect recursive calls in a clone correctly

login
register
mail settings
Submitter Aldy Hernandez
Date Feb. 16, 2011, 3:19 p.m.
Message ID <4D5BEAEA.2020102@redhat.com>
Download mbox | patch
Permalink /patch/83371/
State New
Headers show

Comments

Aldy Hernandez - Feb. 16, 2011, 3:19 p.m.
> (2) Make is_tm_ending_fndecl imply is_ctrl_altering_stmt to force the
>      basic block to end.  We're eventually going to split the block there
>      during pass_tm_edges, but before that we don't have another edge to
>      drop in.  IF there's no other fallout from this change, it's probably
>      the easiest change to make.  I'm not sure why else I wouldn't have
>      done this in the first place, except for the lack-of-an-edge thing.

Almost.  This almost fixes the problem, until I realized that if I 
change the function call to another (and not recurse), we were still 
creating an unnecessary clone.  This is the problem reported in PR47689.

It turns out the bb_in_TM_region map was not stopping at exit blocks, 
and including BB's past the commit.  So in addition to your suggestion, 
we need to stop at exit blocks when accumulating the bb_in_TM_region bitmap.

The patch below fixes everything (including PR47689), cleans up things, 
and is simpler than the last revision.  Yay.  Twofer patch!

Ok for branch?
PR 47690
	* trans-mem.c (ipa_tm_transform_calls_1): Abstract edge
	redirection logic...
	(ipa_tm_transform_calls_redirect): ...here.  Redirect recursive
	clone calls even if we have scanned past the end of the
	transaction.
	(call_past_tm_end): New.
	(ipa_tm_scan_calls_transaction): Make sure callee is really inside
	a transaction.
	(ipa_tm_note_irrevocable): Same.
	(ipa_tm_execute): Do not scan past exit blocks when accumulating BB's.
	* tree-cfg.c (is_ctrl_altering_stmt): Add TM ending statements.
	* tree.h (is_tm_ending_fndecl): New prototype.

Patch

Index: tree.h
===================================================================
--- tree.h	(revision 170050)
+++ tree.h	(working copy)
@@ -5393,6 +5393,7 @@  extern tree build_tm_abort_call (locatio
 extern bool is_tm_safe (const_tree);
 extern bool is_tm_pure (const_tree);
 extern bool is_tm_may_cancel_outer (tree);
+extern bool is_tm_ending_fndecl (tree);
 extern void record_tm_replacement (tree, tree);
 extern void tm_malloc_replacement (tree);
 
Index: testsuite/gcc.dg/tm/pr47690.c
===================================================================
--- testsuite/gcc.dg/tm/pr47690.c	(revision 0)
+++ testsuite/gcc.dg/tm/pr47690.c	(revision 0)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm" } */
+
+int george;
+
+__attribute__((transaction_callable))
+void q2()
+{
+  __transaction [[atomic]]
+    {
+      george=999;
+    }
+  q2();
+}
Index: testsuite/gcc.dg/tm/pr47690-2.c
===================================================================
--- testsuite/gcc.dg/tm/pr47690-2.c	(revision 0)
+++ testsuite/gcc.dg/tm/pr47690-2.c	(revision 0)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm" } */
+
+int george;
+
+void q1()
+{
+  __transaction [[atomic]] {
+      george=999;
+  }
+  q1();
+}
+
+/* { dg-final { scan-assembler-not "ZGTt2q1" } } */
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 170142)
+++ trans-mem.c	(working copy)
@@ -292,7 +292,7 @@  is_transactional_stmt (const_gimple stmt
 
 /* Return true for built in functions that "end" a transaction.   */
 
-static bool
+bool
 is_tm_ending_fndecl (tree fndecl)
 {
   if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
@@ -4279,6 +4279,89 @@  ipa_tm_insert_gettmclone_call (struct cg
   return true;
 }
 
+/* Helper function for ipa_tm_transform_calls*.  Given a call
+   statement in GSI which resides inside transaction REGION, redirect
+   the call to either its wrapper function, or its clone.
+   PAST_TM_ENDING_STMT is true if we have scanned past the end of one
+   of the TM ending statements.  */
+
+static void
+ipa_tm_transform_calls_redirect (struct cgraph_node *node,
+				 struct tm_region *region,
+				 gimple_stmt_iterator *gsi,
+				 bool past_tm_ending_stmt,
+				 bool *need_ssa_rename_p)
+{
+  gimple stmt = gsi_stmt (*gsi);
+  struct cgraph_node *new_node;
+  struct cgraph_edge *e = cgraph_edge (node, stmt);
+  tree fndecl = gimple_call_fndecl (stmt);
+
+  /* We only redirect calls inside of a transaction, with the
+     exception of recursive calls to the clone itself.  We must make
+     sure recursive calls in a cloned function get redirected to the
+     clone itself, not the original function.  So... bail unless we're
+     inside a transaction or we're handling this special case.  */
+  if (past_tm_ending_stmt
+      && (e->caller != e->callee
+	  || !DECL_IS_TM_CLONE (e->callee->decl)))
+    return;
+
+  /* For a recursive call to the clone, call the clone directly.  No
+     need to go through the runtime...  */
+  if (e->caller == e->callee
+      && DECL_IS_TM_CLONE (e->callee->decl))
+    fndecl = e->callee->decl;
+  else
+    {
+      /* ...otherwise start looking for a replacement.  */
+      fndecl = find_tm_replacement_function (fndecl);
+    }
+
+  /* If there is a replacement, use it, otherwise use the clone.  */
+  if (fndecl)
+    {
+      new_node = cgraph_node (fndecl);
+
+      /* ??? Mark all transaction_wrap functions tm_may_enter_irr.
+
+	 We can't do this earlier in record_tm_replacement because
+	 cgraph_remove_unreachable_nodes is called before we inject
+	 references to the node.  Further, we can't do this in some
+	 nice central place in ipa_tm_execute because we don't have
+	 the exact list of wrapper functions that would be used.
+	 Marking more wrappers than necessary results in the creation
+	 of unnecessary cgraph_nodes, which can cause some of the
+	 other IPA passes to crash.
+
+	 We do need to mark these nodes so that we get the proper
+	 result in expand_call_tm.  */
+      new_node->local.tm_may_enter_irr = 1;
+    }
+  else
+    {
+      struct tm_ipa_cg_data *d = get_cg_data (e->callee);
+      new_node = d->clone;
+
+      /* As we've already skipped pure calls and appropriate builtins,
+	 and we've already marked irrevocable blocks, if we can't come
+	 up with a static replacement, then ask the runtime.  */
+      if (new_node == NULL)
+	{
+	  *need_ssa_rename_p |=
+	    ipa_tm_insert_gettmclone_call (node, region, gsi, stmt);
+	  cgraph_remove_edge (e);
+	  return;
+	}
+
+      fndecl = new_node->decl;
+    }
+
+  cgraph_redirect_edge_callee (e, new_node);
+  gimple_call_set_fndecl (stmt, fndecl);
+  return;
+}
+
 /* Helper function for ipa_tm_transform_calls.  For a given BB,
    install calls to tm_irrevocable when IRR_BLOCKS are reached,
    redirect other calls to the generated transactional clone.  */
@@ -4289,6 +4372,7 @@  ipa_tm_transform_calls_1 (struct cgraph_
 {
   gimple_stmt_iterator gsi;
   bool need_ssa_rename = false;
+  bool past_tm_ending_stmt = false;
 
   if (irr_blocks && bitmap_bit_p (irr_blocks, bb->index))
     {
@@ -4299,8 +4383,6 @@  ipa_tm_transform_calls_1 (struct cgraph_
   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
     {
       gimple stmt = gsi_stmt (gsi);
-      struct cgraph_edge *e;
-      struct cgraph_node *new_node;
       tree fndecl;
 
       if (!is_gimple_call (stmt))
@@ -4310,61 +4392,26 @@  ipa_tm_transform_calls_1 (struct cgraph_
 
       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;
-      e = cgraph_edge (node, stmt);
-
-      /* If there is a replacement, use it, otherwise use the clone.  */
-      fndecl = find_tm_replacement_function (fndecl);
-      if (fndecl)
+      if (!past_tm_ending_stmt)
 	{
-	  new_node = cgraph_node (fndecl);
-
-	  /* ??? Mark all transaction_wrap functions tm_may_enter_irr.
-
-	     We can't do this earlier in record_tm_replacement because
-	     cgraph_remove_unreachable_nodes is called before we inject
-	     references to the node.  Further, we can't do this in some
-	     nice central place in ipa_tm_execute because we don't have
-	     the exact list of wrapper functions that would be used.
-	     Marking more wrappers than necessary results in the creation
-	     of unnecessary cgraph_nodes, which can cause some of the
-	     other IPA passes to crash.
-
-	     We do need to mark these nodes so that we get the proper
-	     result in expand_call_tm.  */
-	  new_node->local.tm_may_enter_irr = 1;
-	}
-      else
-	{
-	  struct tm_ipa_cg_data *d = get_cg_data (e->callee);
-	  new_node = d->clone;
-
-	  /* As we've already skipped pure calls and appropriate builtins,
-	     and we've already marked irrevocable blocks, if we can't come
-	     up with a static replacement, then ask the runtime.  */
-	  if (new_node == NULL)
+	  /* For indirect calls, pass the address through the runtime.  */
+	  if (fndecl == NULL)
 	    {
 	      need_ssa_rename |=
-	        ipa_tm_insert_gettmclone_call (node, region, &gsi, stmt);
-	      cgraph_remove_edge (e);
+		ipa_tm_insert_gettmclone_call (node, region, &gsi, stmt);
 	      continue;
 	    }
 
-	  fndecl = new_node->decl;
+	  if (is_tm_ending_fndecl (fndecl))
+	    past_tm_ending_stmt = true;
 	}
 
-      cgraph_redirect_edge_callee (e, new_node);
-      gimple_call_set_fndecl (stmt, fndecl);
+      /* Redirect edges to the appropriate replacement or clone.  This
+	 will even redirect calls outside of a transaction, but only
+	 if they're recursive calls to the clone.  */
+      ipa_tm_transform_calls_redirect (node, region, &gsi,
+				       past_tm_ending_stmt,
+				       &need_ssa_rename);
     }
 
   return need_ssa_rename;
@@ -4500,7 +4547,7 @@  ipa_tm_execute (void)
   bitmap_obstack_initialize (&tm_obstack);
   bb_in_TM_region = BITMAP_ALLOC (&tm_obstack);
 
-  /* Build a bitmap of all BB"s inside transaction regions.  */
+  /* Build a bitmap of all BB's inside transaction regions.  */
   for (node = cgraph_nodes; node; node = node->next)
     if (node->reachable && node->lowered
 	&& cgraph_function_body_availability (node) >= AVAIL_OVERWRITABLE)
@@ -4509,7 +4556,9 @@  ipa_tm_execute (void)
 	regions = ipa_tm_region_init (node); 
 	for ( ; regions; regions = regions->next)
 	  {
-	    get_tm_region_blocks (regions->entry_block, NULL, NULL, 
+	    get_tm_region_blocks (regions->entry_block,
+				  regions->exit_blocks,
+				  NULL, 
 				  bb_in_TM_region, false);
 	  }
       }
Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 170050)
+++ tree-cfg.c	(working copy)
@@ -2253,6 +2253,13 @@  is_ctrl_altering_stmt (gimple t)
 	/* A call also alters control flow if it does not return.  */
 	if (flags & ECF_NORETURN)
 	  return true;
+
+	/* TM ending statements have backedges out of the transaction.
+	   Return true so we split the basic block containing
+	   them.  */
+	if (flags & ECF_TM_OPS
+	    && is_tm_ending_fndecl (gimple_call_fndecl (t)))
+	  return true;
       }
       break;