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. 10, 2010, 9:43 p.m.
Message ID <20101110214350.GA15556@redhat.com>
Download mbox | patch
Permalink /patch/70693/
State New
Headers show

Comments

Aldy Hernandez - Nov. 10, 2010, 9:43 p.m.
The following patch fixes the referenced PR.  The problem is an ICE on
an asm statement brought in by inlining.  Richard suggested we go into
serial irrevocable before the asm.

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46269
> 
> --- Comment #4 from Richard Henderson <rth at gcc dot gnu.org> 2010-11-09 18:06:35 UTC ---
> Since updateBuildingSite is transaction_callable, not 
> transaction_safe, we should handle this no matter how
> the other functions are annotated.
> 
> When atomic_exchange_and_add is not annotated, we should
> transition to serial-irrevocable mode before the asm,
> just as we would do when calling any other unknown function.
> 
> When atomic_exchange_and_add is annotated pure, we should
> believe it and not process anything inside.  We do not
> currently support a general TM escape mechanism on a per-
> block basis, so the only way to really ignore stuff inside
> transaction_pure functions is to *not* inline them.

That sounds too easy.  You mean like this?

I didn't update the edges because I see we don't do so either for the
GIMPLE_CALL case above (in expand_call_tm).  I always get confused
whether I should update the edges or not.

Tested on x86-64 Linux.  Ok for branch?

	PR/46269
	* trans-mem.c (expand_block_tm): Handle GIMPLE_ASM.
Richard Henderson - Nov. 11, 2010, 4:02 p.m.
On 11/10/2010 01:43 PM, Aldy Hernandez wrote:
>  	case GIMPLE_ASM:
> -	  gcc_unreachable ();
> +	  {
> +	    gimple g;
> +	    g = gimple_build_call (built_in_decls[BUILT_IN_TM_IRREVOCABLE], 0);
> +	    gimple_set_location (g, gimple_location (stmt));
> +	    gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> +	    transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE);
> +	    break;

The reason there's a gcc_unreachable there is that we're supposed to
have already handled this case.

See ipa_tm_scan_irr_block where we notice the asm, and ipa_tm_scan_irr_blocks
where we mark the block as irrevocable.  See ipa_tm_transform_calls_1 where
we insert the irrevocable call at the head of blocks so marked.

In pass execute_tm_mark, we're supposed to have stopped transforming the
region when the block is irrevocable.  Thus the /*stop_at_irr_p=*/true in
the call to get_tm_region_blocks.  Apparently, that isn't doing what it says,
so at a guess that's where the real bug lies.  Assuming, of course, that we
really did insert the irrevocable call during the ipa pass as we intended.


r~
Aldy Hernandez - Nov. 11, 2010, 8:02 p.m.
On Thu, Nov 11, 2010 at 08:02:50AM -0800, Richard Henderson wrote:
> On 11/10/2010 01:43 PM, Aldy Hernandez wrote:
> >  	case GIMPLE_ASM:
> > -	  gcc_unreachable ();
> > +	  {
> > +	    gimple g;
> > +	    g = gimple_build_call (built_in_decls[BUILT_IN_TM_IRREVOCABLE], 0);
> > +	    gimple_set_location (g, gimple_location (stmt));
> > +	    gsi_insert_before (&gsi, g, GSI_SAME_STMT);
> > +	    transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE);
> > +	    break;
> 
> The reason there's a gcc_unreachable there is that we're supposed to
> have already handled this case.
> 
> See ipa_tm_scan_irr_block where we notice the asm, and ipa_tm_scan_irr_blocks
> where we mark the block as irrevocable.  See ipa_tm_transform_calls_1 where
> we insert the irrevocable call at the head of blocks so marked.

Actually, that had been my first approach, but ipa_tm_scan_irr_function
(and consequently ipa_tm_scan_irr_block*) only gets called for items in
the worklist.  And the worklist is only populated for:

      /* 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)))
	ipa_tm_note_irrevocable (node, &worklist);

Since updateBuildingSite() is not makred as irrevocable,
is_tm_irrevoable() returns false, and we never do the
ipa_tm_scan_irr_block() thing you expect.

I thought about teaching is_tm_irrevocable() to look at the
tm_may_enter_irr bit you set in the IPA diagnose pass, but when that
runs, we haven't run the inliner so we don't see the asm statement.

Aldy
Richard Henderson - Nov. 11, 2010, 8:14 p.m.
On 11/11/2010 12:02 PM, Aldy Hernandez wrote:
> Actually, that had been my first approach, but ipa_tm_scan_irr_function
> (and consequently ipa_tm_scan_irr_block*) only gets called for items in
> the worklist.  And the worklist is only populated for:
> 
>       /* 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)))
> 	ipa_tm_note_irrevocable (node, &worklist);
...
> Since updateBuildingSite() is not makred as irrevocable,
> is_tm_irrevoable() returns false, and we never do the
> ipa_tm_scan_irr_block() thing you expect.

Um, no.  This is for marking entire functions irrevocable.

When the entire function is irrevocable, of course we don't
scan the blocks.  When the entire function is NOT so marked,
then we scan the blocks looking for paths within the function
that need to transition to serial-irrevocable.


r~

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,30 @@ 
+// { 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 166496)
+++ trans-mem.c	(working copy)
@@ -2196,7 +2196,14 @@  expand_block_tm (struct tm_region *regio
 	  break;
 
 	case GIMPLE_ASM:
-	  gcc_unreachable ();
+	  {
+	    gimple g;
+	    g = gimple_build_call (built_in_decls[BUILT_IN_TM_IRREVOCABLE], 0);
+	    gimple_set_location (g, gimple_location (stmt));
+	    gsi_insert_before (&gsi, g, GSI_SAME_STMT);
+	    transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE);
+	    break;
+	  }
 
 	default:
 	  break;