Patchwork [trans-mem] PR46270: handle throwing statements

login
register
mail settings
Submitter Aldy Hernandez
Date Nov. 19, 2010, 5:18 p.m.
Message ID <4CE6B182.4040106@redhat.com>
Download mbox | patch
Permalink /patch/72276/
State New
Headers show

Comments

Aldy Hernandez - Nov. 19, 2010, 5:18 p.m.
On 11/19/10 11:26, Richard Henderson wrote:
>> +      if (!gimple_call_nothrow_p (stmt)
>> +	&&  gsi_one_before_end_p (*gsi))
>>      
> As I said before: stmt_can_throw_internal.  You're doing
> unnecessary splitting in the case the call *can* throw,
> but there's no handler within the current function.
>    

Poop, missed that.  Thanks.
>> +	  gsi_insert_on_edge_immediate (fallthru_edge, stmt);
>> +
>> +	  /* Split the block so the instrumentation code resides in
>> +	     it's own separate BB.  Then we can mark this BB as handled
>> +	     and avoid rescanning it again.  */
>> +	  bb = gimple_bb (stmt);
>> +	  split_block (bb, stmt);
>> +	  *gsi = gsi_start_bb (bb);
>>      
> Splitting the block is pointless.  If you don't want to process
>    
Watcha talking about Willis?  If I split it, it ends up all by itself, 
and I can mark it quite nicely.  Not that it's elegant, or anything...  
Just curious, why is it pointless?

> the statement, and you're worried about it being placed into a
> block that you've not yet processed, then use the non-immediate
> form of edge insertion and then use gsi_commit_edge_inserts at
> the end.
>
> Normally when we do that, we also accumulate a flag that says
> whether calling gsi_commit_edge_inserts is necessary.
>    

Sigh, I'm beginning to see that there is no elegant solution.  Yuck, one 
more flag.

This is all nasty, but here it is...

OK?
PR/46270
	* trans-mem.c (struct tm_region): Add pending_edge_inserts_p.
	(tm_region_init_0): Initialize pending_edge_inserts_p.
	(expand_call_tm): Handle throwing calls.
	(execute_tm_mark): Handle clones and transactions the same.
	Flush pending statements if necessary.
Richard Henderson - Nov. 19, 2010, 5:47 p.m.
> +  region->pending_edge_inserts_p = false;

You don't want to put this flag on the region.  You want to do this
edge insertion once per function, not once per region.  I.e. outside
that big loop in execute_tm_mark.

The easiest thing is to make the flag file-scope static, cleared at
the beginning of execute_tm_mark.

Ok with that change.


r~

Patch

Index: testsuite/g++.dg/tm/pr46270.C
===================================================================
--- testsuite/g++.dg/tm/pr46270.C	(revision 0)
+++ testsuite/g++.dg/tm/pr46270.C	(revision 0)
@@ -0,0 +1,27 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm" }
+
+#include <list>
+class Game
+{
+public:
+  struct BuildProject
+  {
+    int posX;
+  };
+  std::list<BuildProject> buildProjects;
+};
+
+static Game game;
+static std::list<std::list<Game::BuildProject>::iterator> erasableBuildProjects;
+
+static void *buildProjectSyncStepConcurrently(int id, int localTeam)
+{
+  __transaction [[relaxed]] {
+    std::list<std::list<Game::BuildProject>::iterator>::iterator it
+      = erasableBuildProjects.begin();
+    game.buildProjects.erase( (std::list<Game::BuildProject> 
+			       ::iterator) *it);
+  }
+  return 0;
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 166872)
+++ trans-mem.c	(working copy)
@@ -1678,6 +1678,9 @@  struct tm_region
 
   /* The set of all blocks that have an TM_IRREVOCABLE call.  */
   bitmap irr_blocks;
+
+  /* True if there are pending edge statements to be committed.  */
+  bool pending_edge_inserts_p;
 };
 
 static struct tm_region *all_tm_regions;
@@ -1707,6 +1710,7 @@  tm_region_init_0 (struct tm_region *oute
     }
   region->inner = NULL;
   region->outer = outer;
+  region->pending_edge_inserts_p = false;
 
   region->transaction_stmt = stmt;
 
@@ -2173,13 +2177,44 @@  expand_call_tm (struct tm_region *region
     {
       tree tmp = make_rename_temp (TREE_TYPE (lhs), NULL);
       location_t loc = gimple_location (stmt);
+      edge fallthru_edge = NULL;
+
+      /* Remember if the call was going to throw.  */
+      if (stmt_can_throw_internal (stmt))
+	{
+	  edge_iterator ei;
+	  edge e;
+	  basic_block bb = gimple_bb (stmt);
+
+	  FOR_EACH_EDGE (e, ei, bb->succs)
+	    if (e->flags & EDGE_FALLTHRU)
+	      {
+		fallthru_edge = e;
+		break;
+	      }
+	}
 
       gimple_call_set_lhs (stmt, tmp);
       update_stmt (stmt);
       stmt = gimple_build_assign (lhs, tmp);
       gimple_set_location (stmt, loc);
-      gsi_insert_after (gsi, stmt, GSI_CONTINUE_LINKING);
-      expand_assign_tm (region, gsi);
+
+      /* We cannot throw in the middle of a BB.  If the call was going
+	 to throw, place the instrumentation on the fallthru edge, so
+	 the call remains the last statement in the block.  */
+      if (fallthru_edge)
+	{
+	  gimple_seq fallthru_seq = gimple_seq_alloc_with_stmt (stmt);
+	  gimple_stmt_iterator fallthru_gsi = gsi_start (fallthru_seq);
+	  expand_assign_tm (region, &fallthru_gsi);
+	  gsi_insert_seq_on_edge (fallthru_edge, fallthru_seq);
+	  region->pending_edge_inserts_p = true;
+	}
+      else
+	{
+	  gsi_insert_after (gsi, stmt, GSI_CONTINUE_LINKING);
+	  expand_assign_tm (region, gsi);
+	}
 
       transaction_subcode_ior (region, GTMA_HAVE_STORE);
     }
@@ -2304,28 +2339,18 @@  execute_tm_mark (void)
 	  else
 	    subcode &= GTMA_DECLARATION_MASK;
 	  gimple_transaction_set_subcode (region->transaction_stmt, subcode);
-
-	  queue = get_tm_region_blocks (region->entry_block,
-					region->exit_blocks,
-					region->irr_blocks,
-					/*stop_at_irr_p=*/true);
-	  for (i = 0; VEC_iterate (basic_block, queue, i, bb); ++i)
-	    expand_block_tm (region, bb);
-	  VEC_free (basic_block, heap, queue);
-	}
-      else
-	/* ...otherwise, we're a clone and the entire function is the
-	   region.  */
-	{
-	  FOR_EACH_BB (bb)
-	    {
-	      /* Stop at irrevocable blocks.  */
-	      if (region->irr_blocks
-		  && bitmap_bit_p (region->irr_blocks, bb->index))
-		break;
-	      expand_block_tm (region, bb);
-	    }
 	}
+
+      queue = get_tm_region_blocks (region->entry_block,
+				    region->exit_blocks,
+				    region->irr_blocks,
+				    /*stop_at_irr_p=*/true);
+      for (i = 0; VEC_iterate (basic_block, queue, i, bb); ++i)
+	expand_block_tm (region, bb);
+      VEC_free (basic_block, heap, queue);
+      if (region->pending_edge_inserts_p)
+	gsi_commit_edge_inserts ();
+
       tm_log_emit ();
     }