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

login
register
mail settings
Submitter Aldy Hernandez
Date Nov. 17, 2010, 4:17 p.m.
Message ID <20101117161707.GA15094@redhat.com>
Download mbox | patch
Permalink /patch/71581/
State New
Headers show

Comments

Aldy Hernandez - Nov. 17, 2010, 4:17 p.m.
On Fri, Nov 12, 2010 at 10:13:02AM -0800, Richard Henderson wrote:
> 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,

I won't undignify myself with a response to the rest of this email.
Suffice to say that I'm an idiot.  I should've quit while I was ahead
and left the implementation to the interested reader...

> 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);

This was actually the root of most everything.  No need to initialize
the worklist, if we will actually fill it with correct info in the first
place :).

Of course, this opened up all sorts of grief.  For instance, we can't
just mark not available functions as irrevocable, because they may be
explicitly marked as safe or pure.  On the other hand, callers of
(assumed) irrevocable functions should not force the irrevocability bits
up the call chain, if the (assumed) irrevocable function was
specifically marked as safe/pure.  The latter problem can be seen in
c-c++-common/tm/safe-3.c.

Anyways, look at the patch now.  Feel free to continue crucifying me if
I got it wrong again.

Tested on x86-64 Linux.

	* gimple-pretty-print.c (dump_gimple_call): Add clone attribute to
	dump.
	* trans-mem.c (tm_region_init_1): Proceed even if there are not
	exit_blocks.
	(gate_tm_init): Initialize irr_blocks.
	(execute_tm_mark): Stop at irrevocable blocks for clones too.
	(ipa_tm_note_irrevocable): Do not mark callers as irrevocable if
	they are marked safe or pure.
	(ipa_tm_scan_irr_function): Only dump irrevocable blocks if we
	have them.
	(ipa_tm_execute): Rework irrevocable marking logic.
	PR/46269
	* trans-mem.c (expand_block_tm): Handle GIMPLE_ASM.
Richard Henderson - Nov. 17, 2010, 4:33 p.m.
On 11/17/2010 08:17 AM, Aldy Hernandez wrote:
> 	* gimple-pretty-print.c (dump_gimple_call): Add clone attribute to
> 	dump.
> 	* trans-mem.c (tm_region_init_1): Proceed even if there are not
> 	exit_blocks.
> 	(gate_tm_init): Initialize irr_blocks.
> 	(execute_tm_mark): Stop at irrevocable blocks for clones too.
> 	(ipa_tm_note_irrevocable): Do not mark callers as irrevocable if
> 	they are marked safe or pure.
> 	(ipa_tm_scan_irr_function): Only dump irrevocable blocks if we
> 	have them.
> 	(ipa_tm_execute): Rework irrevocable marking logic.

Ok with...

> 	* trans-mem.c (expand_block_tm): Handle GIMPLE_ASM.

Leftover?

> +static inline void atomic_exchange_and_add()
> +{   
> +  __asm__  ("blah");
> +}

Just use "" so that the file will assemble everywhere.


r~
Aldy Hernandez - Nov. 17, 2010, 4:40 p.m.
>> +static inline void atomic_exchange_and_add()
>> +{
>> +  __asm__  ("blah");
>> +}
>>      
> Just use "" so that the file will assemble everywhere.
>    

I thought the gcc.dg machinery didn't assemble, just compiled?

In any case... I'm quitting while I'm ahead.  Committing and closing PR.

Thanks.
Mike Stump - Nov. 17, 2010, 4:56 p.m.
On Nov 17, 2010, at 8:40 AM, Aldy Hernandez wrote:
> I thought the gcc.dg machinery didn't assemble, just compiled?

assemble means assemble, run, means run, compile, means link to a .o.

> +// { dg-do compile }

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: gimple-pretty-print.c
===================================================================
--- gimple-pretty-print.c	(revision 166496)
+++ gimple-pretty-print.c	(working copy)
@@ -541,6 +541,8 @@  dump_gimple_call (pretty_printer *buffer
   /* Dump the arguments of _ITM_beginTransaction sanely.  */
   if (TREE_CODE (fn) == ADDR_EXPR)
     fn = TREE_OPERAND (fn, 0);
+  if (TREE_CODE (fn) == FUNCTION_DECL && DECL_IS_TM_CLONE (fn))
+    pp_string (buffer, " [tm-clone]");
   if (TREE_CODE (fn) == FUNCTION_DECL
       && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL
       && DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_START
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 
@@ -1746,8 +1747,9 @@  tm_region_init_1 (struct tm_region *regi
 	  tree fn = gimple_call_fndecl (g);
 	  if (fn && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL)
 	    {
-	      if (DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT
-		  || DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT_EH)
+	      if ((DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT
+		   || DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT_EH)
+		  && region->exit_blocks)
 		{
 		  bitmap_set_bit (region->exit_blocks, bb->index);
 		  region = region->outer;
@@ -1831,6 +1833,10 @@  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);
+      /* For a clone, the entire function is the region.  But even if
+	 we don't need to record any exit blocks, we may need to
+	 record irrevocable blocks.  */
+      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 ();
     }
@@ -3452,6 +3467,11 @@  ipa_tm_note_irrevocable (struct cgraph_n
       /* Don't examine recursive calls.  */
       if (e->caller == node)
 	continue;
+      /* Even if we think we can go irrevocable, believe the user
+	 above all.  */
+      if (is_tm_safe (e->caller->decl)
+	  || is_tm_pure (e->caller->decl))
+	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);
@@ -3714,21 +3734,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;
@@ -4394,12 +4414,19 @@  ipa_tm_execute (void)
 
       /* Some callees cannot be arbitrarily cloned.  These will always be
 	 irrevocable.  Mark these now, so that we need not scan them.  */
-      if (is_tm_irrevocable (node->decl)
-	  || (a >= AVAIL_OVERWRITABLE
-	      && !tree_versionable_function_p (node->decl)))
+      if (is_tm_irrevocable (node->decl))
+	ipa_tm_note_irrevocable (node, &worklist);
+      else if (a <= AVAIL_NOT_AVAILABLE
+	       && !is_tm_safe (node->decl)
+	       && !is_tm_pure (node->decl))
 	ipa_tm_note_irrevocable (node, &worklist);
-      else if (a >= AVAIL_OVERWRITABLE && !d->is_irrevocable)
-	ipa_tm_scan_calls_clone (node, &tm_callees);
+      else if (a >= AVAIL_OVERWRITABLE)
+	{
+	  if (!tree_versionable_function_p (node->decl))
+	    ipa_tm_note_irrevocable (node, &worklist);
+	  else if (!d->is_irrevocable)
+	    ipa_tm_scan_calls_clone (node, &tm_callees);
+	}
     }
 
   /* Iterate scans until no more work to be done.  Prefer not to use