diff mbox

[Bug,c++/46269,trans-mem] internal compiler error in expand_block_tm of trans-mem.c

Message ID 20101112142409.GA8529@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Nov. 12, 2010, 2:24 p.m. UTC 
> Um, no.  This is for marking entire functions irrevocable.

Um, no to your no. :).

As discussed on irc, we're not even scanning the functions because the
worklist wasn't being seeded correctly.  Fixing this unearth a bunch of
additional problems (of course :)).

1. We weren't stopping at irrevocable blocks for cloned functions.  We
were cruising the whole way through the basic blocks.

2. We weren't gathering any irrevocable bits for cloned functions.
So there was no way to stop, even if #1 was fixed.

3. We weren't seeding the list that contained the functions to later
scan and insert irrevocable mode changes.

4. The IPA dump in ipa_tm_scan_irr_function() was trying to dump
irrevocable bits even if we had just freed them :).

All of this, and more, for a limited time, and fixed in the following
patch!

Tested on x86-64 Linux.

OK for branch?

	* trans-mem.c (tm_region_init_1): Mark blocks with GIMPLE_ASMs as
	irrevocable.
	(gate_tm_init): Initialize irr_blocks.
	(execute_tm_mark): Stop at irrevocable blocks for clones too.
	(maybe_push_queue): Add to queue2_p as well.
	(ipa_tm_scan_calls_transaction): Pass new argument to
	maybe_push_queue.
	(ipa_tm_scan_calls_clone): New argument.  Pass new argument to
	maybe_push_queue.
	(ipa_tm_note_irrevocable): Pass new argument to maybe_push_queue.
	(ipa_tm_execute): Same.
	(ipa_tm_scan_irr_function): Skip functions with no
	DECL_STRUCT_FUNCTION.
	Only dump irrevocable blocks if we have them.
	PR/46269
	* trans-mem.c (expand_block_tm): Handle GIMPLE_ASM.

Comments

Richard Henderson Nov. 12, 2010, 6:13 p.m. UTC | #1 
On 11/12/2010 06:24 AM, Aldy Hernandez wrote:
> -      if (gimple_code (g) == GIMPLE_CALL)
> +      if (gimple_code (g) == GIMPLE_ASM && region->irr_blocks)
> +	bitmap_set_bit (region->irr_blocks, bb->index);

This is wrong.  An ASM does not transition to irrevocable,
the call to BUILT_IN_TM_IRREVOCABLE that we insertted before
the ASM does that.

This function ought to be collecting what IS, as distinguished
from the ipa pass which ought to collect what SHOULD BE.

> +      /* A clone may have irrevocable blocks (i.e. GIMPLE_ASM) even if
> +	 we're not technically inside a transaction.  */

The comment is badly worded.  A clone by definition means we ARE
inside of a transaction, just one that starts somewhere up the
dynamic call chain.

> +/* Add NODE to the end of QUEUE, unless IN_QUEUE_P indicates that it
> +   is already present.  Also adds NODE to the end of QUEUE2, but only
> +   if QUEUE2 is non-NULL.  */
>  
>  static void
>  maybe_push_queue (struct cgraph_node *node,
> -		  cgraph_node_queue *queue_p, bool *in_queue_p)
> +		  cgraph_node_queue *queue_p,
> +		  cgraph_node_queue *queue2_p,
> +		  bool *in_queue_p)

Err, really?  Manipulating two queues at once?  Why?

This seems wrong, since there ought to be two in_queue_p variables
involved, at which point what's wrong with two calls to this function?

> @@ -3659,6 +3682,10 @@ ipa_tm_scan_irr_function (struct cgraph_
>    VEC (basic_block, heap) *queue;
>    bool ret = false;
>  
> +  /* Skip things like new/delete operators.  */
> +  if (!DECL_STRUCT_FUNCTION (node->decl))
> +    return false;

This is a bug elsewhere.  E.g.

>       if (is_tm_irrevocable (node->decl)
>           || (a >= AVAIL_OVERWRITABLE
>               && !tree_versionable_function_p (node->decl)))
>         ipa_tm_note_irrevocable (node, &worklist);

  if (is_tm_irrevocable (node->decl)
      || a <= AVAIL_NOT_AVAILABLE
      || !tree_versionable_function_p (node->decl))
    ipa_tm_note_irrevocable (node, &worklist);

> +  worklist = VEC_copy (cgraph_node_p, heap, tm_callees);

Don't you have to set in_worklist for each of these?


r~
diff mbox

Patch

Index: testsuite/g++.dg/tm/pr46269.C
===================================================================
--- testsuite/g++.dg/tm/pr46269.C	(revision 0)
+++ testsuite/g++.dg/tm/pr46269.C	(revision 0)
@@ -0,0 +1,29 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm" }
+
+static inline void atomic_exchange_and_add()
+{   
+  __asm__  ("blah");
+}
+
+template<class T> class shared_ptr
+{
+public:
+  shared_ptr( T * p )
+  { 
+    atomic_exchange_and_add();
+  }
+};
+
+class BuildingCompletedEvent
+{ 
+  public:
+  __attribute__((transaction_callable)) void updateBuildingSite(void);
+  __attribute__((transaction_pure)) BuildingCompletedEvent();
+};
+
+void BuildingCompletedEvent::updateBuildingSite(void)
+{ 
+  shared_ptr<BuildingCompletedEvent> event(new BuildingCompletedEvent());
+}
+
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 166604)
+++ trans-mem.c	(working copy)
@@ -1732,7 +1732,8 @@  tm_region_init_1 (struct tm_region *regi
   gimple_stmt_iterator gsi;
   gimple g;
 
-  if (!region || !region->exit_blocks)
+  if (!region
+      || (!region->irr_blocks && !region->exit_blocks))
     return region;
 
   /* Check to see if this is the end of a region by seeing if it 
@@ -1741,7 +1742,9 @@  tm_region_init_1 (struct tm_region *regi
   for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
     {
       g = gsi_stmt (gsi);
-      if (gimple_code (g) == GIMPLE_CALL)
+      if (gimple_code (g) == GIMPLE_ASM && region->irr_blocks)
+	bitmap_set_bit (region->irr_blocks, bb->index);
+      else if (gimple_code (g) == GIMPLE_CALL && region->exit_blocks)
 	{
 	  tree fn = gimple_call_fndecl (g);
 	  if (fn && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL)
@@ -1831,6 +1834,9 @@  gate_tm_init (void)
 	obstack_alloc (&tm_obstack.obstack, sizeof (struct tm_region));
       memset (region, 0, sizeof (*region));
       region->entry_block = single_succ (ENTRY_BLOCK_PTR);
+      /* A clone may have irrevocable blocks (i.e. GIMPLE_ASM) even if
+	 we're not technically inside a transaction.  */
+      region->irr_blocks = BITMAP_ALLOC (&tm_obstack);
 
       tm_region_init (region);
     }
@@ -2285,6 +2291,7 @@  execute_tm_mark (void)
   for (region = all_tm_regions; region ; region = region->next)
     {
       tm_log_init ();
+      /* If we have a transaction...  */
       if (region->exit_blocks)
 	{
 	  unsigned int subcode
@@ -2307,9 +2314,17 @@  execute_tm_mark (void)
 	  VEC_free (basic_block, heap, queue);
 	}
       else
+	/* ...otherwise, we're a clone and the entire function is the
+	   region.  */
 	{
 	  FOR_EACH_BB (bb)
-	    expand_block_tm (region, bb);
+	    {
+	      /* Stop at irrevocable blocks.  */
+	      if (region->irr_blocks
+		  && bitmap_bit_p (region->irr_blocks, bb->index))
+		break;
+	      expand_block_tm (region, bb);
+	    }
 	}
       tm_log_emit ();
     }
@@ -3366,17 +3381,22 @@  get_cg_data (struct cgraph_node *node)
   return d;
 }
 
-/* Add NODE to the end of QUEUE, unless IN_QUEUE_P indicates that 
-   it is already present.  */
+/* Add NODE to the end of QUEUE, unless IN_QUEUE_P indicates that it
+   is already present.  Also adds NODE to the end of QUEUE2, but only
+   if QUEUE2 is non-NULL.  */
 
 static void
 maybe_push_queue (struct cgraph_node *node,
-		  cgraph_node_queue *queue_p, bool *in_queue_p)
+		  cgraph_node_queue *queue_p,
+		  cgraph_node_queue *queue2_p,
+		  bool *in_queue_p)
 {
   if (!*in_queue_p)
     {
       *in_queue_p = true;
       VEC_safe_push (cgraph_node_p, heap, *queue_p, node);
+      if (queue2_p != NULL)
+	VEC_safe_push (cgraph_node_p, heap, *queue2_p, node);
     }
 }
 
@@ -3404,16 +3424,18 @@  ipa_tm_scan_calls_transaction (struct cg
 
 	d = get_cg_data (e->callee);
 	d->tm_callers_normal++;
-	maybe_push_queue (e->callee, callees_p, &d->in_callee_queue);
+	maybe_push_queue (e->callee, callees_p, NULL, &d->in_callee_queue);
       }
 }
 
-/* Scan all calls in NODE as if this is the transactional clone,
-   and push the destinations into the callee queue.  */
+/* Scan all calls in NODE as if this is the transactional clone, and
+   push the destinations into the callee queue.  Also, push the
+   destinations into the CALLEES2 queue.  */
 
 static void
-ipa_tm_scan_calls_clone (struct cgraph_node *node, 
-			 cgraph_node_queue *callees_p)
+ipa_tm_scan_calls_clone (struct cgraph_node *node,
+			 cgraph_node_queue *callees_p,
+			 cgraph_node_queue *callees2_p)
 {
   struct cgraph_edge *e;
 
@@ -3428,7 +3450,8 @@  ipa_tm_scan_calls_clone (struct cgraph_n
 	  struct tm_ipa_cg_data *d = get_cg_data (e->callee);
 
 	  d->tm_callers_clone++;
-	  maybe_push_queue (e->callee, callees_p, &d->in_callee_queue);
+	  maybe_push_queue (e->callee, callees_p, callees2_p,
+			    &d->in_callee_queue);
 	}
     }
 }
@@ -3454,7 +3477,7 @@  ipa_tm_note_irrevocable (struct cgraph_n
 	continue;
       if (gimple_call_in_transaction_p (e->call_stmt))
 	d->want_irr_scan_normal = true;
-      maybe_push_queue (e->caller, worklist_p, &d->in_worklist);
+      maybe_push_queue (e->caller, worklist_p, NULL, &d->in_worklist);
     }
 }
 
@@ -3659,6 +3682,10 @@  ipa_tm_scan_irr_function (struct cgraph_
   VEC (basic_block, heap) *queue;
   bool ret = false;
 
+  /* Skip things like new/delete operators.  */
+  if (!DECL_STRUCT_FUNCTION (node->decl))
+    return false;
+
   current_function_decl = node->decl;
   push_cfun (DECL_STRUCT_FUNCTION (node->decl));
   calculate_dominance_info (CDI_DOMINATORS);
@@ -3714,21 +3741,21 @@  ipa_tm_scan_irr_function (struct cgraph_
 	d->irrevocable_blocks_clone = new_irr;
       else
 	d->irrevocable_blocks_normal = new_irr;
+
+      if (dump_file)
+	{
+	  const char *dname;
+	  bitmap_iterator bmi;
+	  unsigned i;
+
+	  dname = lang_hooks.decl_printable_name (current_function_decl, 2);
+	  EXECUTE_IF_SET_IN_BITMAP (new_irr, 0, i, bmi)
+	    fprintf (dump_file, "%s: bb %d goes irrevocable\n", dname, i);
+	}
     }
   else
     BITMAP_FREE (new_irr);
 
-  if (dump_file)
-    {
-      const char *dname;
-      bitmap_iterator bmi;
-      unsigned i;
-
-      dname = lang_hooks.decl_printable_name (current_function_decl, 2);
-      EXECUTE_IF_SET_IN_BITMAP (new_irr, 0, i, bmi)
-	fprintf (dump_file, "%s: bb %d goes irrevocable\n", dname, i);
-    }
-
   VEC_free (basic_block, heap, queue);
   pop_cfun ();
   current_function_decl = NULL;
@@ -4353,7 +4380,7 @@  ipa_tm_execute (void)
 	&& cgraph_function_body_availability (node) >= AVAIL_OVERWRITABLE)
       {
 	d = get_cg_data (node);
-	maybe_push_queue (node, &tm_callees, &d->in_callee_queue);
+	maybe_push_queue (node, &tm_callees, NULL, &d->in_callee_queue);
       }
 
   /* For all local reachable functions...  */
@@ -4386,6 +4413,7 @@  ipa_tm_execute (void)
   /* For every local function on the callee list, scan as if we will be
      creating a transactional clone, queueing all new functions we find
      along the way.  */
+  worklist = VEC_copy (cgraph_node_p, heap, tm_callees);
   for (i = 0; i < VEC_length (cgraph_node_p, tm_callees); ++i)
     {
       node = VEC_index (cgraph_node_p, tm_callees, i);
@@ -4399,7 +4427,7 @@  ipa_tm_execute (void)
 	      && !tree_versionable_function_p (node->decl)))
 	ipa_tm_note_irrevocable (node, &worklist);
       else if (a >= AVAIL_OVERWRITABLE && !d->is_irrevocable)
-	ipa_tm_scan_calls_clone (node, &tm_callees);
+	ipa_tm_scan_calls_clone (node, &tm_callees, &worklist);
     }
 
   /* Iterate scans until no more work to be done.  Prefer not to use
@@ -4438,7 +4466,7 @@  ipa_tm_execute (void)
 	{
 	  d = get_cg_data (node);
 	  gcc_assert (d->in_worklist == false);
-	  maybe_push_queue (node, &worklist, &d->in_worklist);
+	  maybe_push_queue (node, &worklist, NULL, &d->in_worklist);
 	}
     }
 
@@ -4461,7 +4489,7 @@  ipa_tm_execute (void)
       for (e = node->callers; e ; e = e->next_caller)
 	if (!is_tm_safe (e->caller->decl)
 	    && !e->caller->local.tm_may_enter_irr)
-	  maybe_push_queue (e->caller, &worklist, &d->in_worklist);
+	  maybe_push_queue (e->caller, &worklist, NULL, &d->in_worklist);
     }
 
   /* Now validate all tm_safe functions, and all atomic regions in