diff mbox

[trans-mem] PR47606: Add a tm-safe marker for GIMPLE_ASM's

Message ID 4D658567.2030401@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Feb. 23, 2011, 10:08 p.m. UTC 
This is yet another problem with inline assembly in transactions.

The problem here is that when we diagnose the presence of inline 
assembly in diagnose_tm(), inlining has not yet run.  After many 
transformations, we may end up inlining GIMPLE_ASM's into a transaction, 
but we lost the context in which it appeared.

In this testcase we have a GIMPLE_ASM in a transaction_pure function, so 
it should be allowed in a transaction when inlined, instead of ICEing 
when seeing the statement later.

I bit the bullet and added a subcode bit to the GIMPLE_ASM tuple to keep 
track that the ASM appeared in a TM pure function.  Richard, I believe 
you had a similar idea, by looking at the comment I removed below.

This patch fixes the testcase in the original PR, though I found yet 
another GIMPLE_ASM bug when playing around with variants of the 
testcase.  I can attack that next, as it's the same ICE but a different 
problem.

OK for branch?
* gimple.h (gimple_asm_set_tm_safe): New.
	(gimple_asm_tm_safe): New.
	(enum gf_mask): Add GF_ASM_TM_SAFE.
	* trans-mem (DIAG_TM_PURE): New macro.
	(diagnose_tm_1): Mark GIMPLE_ASM's inside a pure function.
	(diagnose_tm_blocks): Set DIAG_TM_PURE accordingly.
	(expand_block_tm): Handle inline asms that appeared in a TM pure
	function.

Comments

Richard Henderson Feb. 24, 2011, 3:58 p.m. UTC | #1 
On 02/23/2011 02:08 PM, Aldy Hernandez wrote:
> +__attribute__((transaction_pure))
> +void atomic_exchange_and_add()
> +{
> +  __asm__ __volatile__     ("");
> +}
> +

Which then immediately fails for one layer of indirection:

inline void atomic_exchange_and_add_1()
{
  asm volatile ("");
}

void __attribute__((transaction_pure))
atomic_exchange_and_add ()
{
  atomic_exchange_and_add_1 ();
}

No, the comment you're referring to adds a new block structure:

  __tm_waiver {
    any_code ();
  }

Which could be implemented with two gimple codes to mark the 
beginning and end of the SEME region.  These can survive all
the way through to the final lowering of all the tm constructs.

If these new gimple codes force the end of a block, then we 
can fairly easily exclude code within this region from the 
blocks returned by get_tm_region_blocks, which hopefully would
Just Work with the rest of the code in trans-mem.c.

I have an idea what's involved, I'll work on this.


r~
Aldy Hernandez Feb. 24, 2011, 7:19 p.m. UTC | #2 
>    __tm_waiver {
>      any_code ();
>    }
>
> Which could be implemented with two gimple codes to mark the
> beginning and end of the SEME region.  These can survive all

BTW, I think you mean SESE (single-entry, single-exit).  It was my 
understanding that explicit control-flow out of a __tm_waiver block was 
prohibited.
Richard Henderson Feb. 24, 2011, 7:24 p.m. UTC | #3 
On 02/24/2011 11:19 AM, Aldy Hernandez wrote:
> 
>>    __tm_waiver {
>>      any_code ();
>>    }
>>
>> Which could be implemented with two gimple codes to mark the
>> beginning and end of the SEME region.  These can survive all
> 
> BTW, I think you mean SESE (single-entry, single-exit).  It was my
> understanding that explicit control-flow out of a __tm_waiver block
> was prohibited.

Meh.  Exception handling edges from the any_code function will at
least require handling 2 edges.  If you have to handle 2, you might
as well handle any amount.

In particular, I plan on implementing this as

  __tm_waiver_begin;
  try {
    any_code ();
  } finally {
    __tm_waiver_end;
  }

Those two gimple codes are markers that tell us what to do whenever
we're traversing the CFG for TM purposes.  They'll get removed when
we finish lowering everything in pass_tm_edges.


r~
diff mbox

Patch

Index: testsuite/g++.dg/tm/pr47606.C
===================================================================
--- testsuite/g++.dg/tm/pr47606.C	(revision 0)
+++ testsuite/g++.dg/tm/pr47606.C	(revision 0)
@@ -0,0 +1,65 @@ 
+// { dg-do compile }
+// { dg-options "-fgnu-tm -O2" }
+
+__attribute__((transaction_pure))
+void atomic_exchange_and_add()
+{
+  __asm__ __volatile__     ("");
+}
+
+template<class X> class sp_counted_impl_p
+{
+ public: 
+  X * px_;
+	
+  void weak_release()
+  {
+    atomic_exchange_and_add ();
+  }
+
+  __attribute__((transaction_safe))
+  void release()
+  {
+    weak_release();
+  }
+
+ sp_counted_impl_p( X * px ): px_( px )
+  {
+  }
+
+  __attribute__((transaction_safe))
+  virtual void dispose()
+  {
+    delete px_;
+  }
+};
+
+template<class Y> class shared_count
+{
+ private:     sp_counted_impl_p<Y> * pi_;
+ public:     shared_count(): pi_(0)
+    {
+    }
+  shared_count( Y * p )
+    {
+      pi_ = new sp_counted_impl_p<Y>( p );
+    }
+  ~shared_count()
+    {
+      pi_->release();
+    }
+};
+
+class Entity;
+
+class GradientInfo
+{
+ public:
+  GradientInfo();
+  shared_count<Entity> sources;
+};
+
+void get_grad()
+{
+  shared_count<GradientInfo>(new GradientInfo());
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 170377)
+++ trans-mem.c	(working copy)
@@ -540,6 +540,7 @@  tm_malloc_replacement (tree from)
 #define DIAG_TM_OUTER		1
 #define DIAG_TM_SAFE		2
 #define DIAG_TM_RELAXED		4
+#define DIAG_TM_PURE		8
 
 struct diagnose_tm
 {
@@ -672,9 +673,6 @@  diagnose_tm_1 (gimple_stmt_iterator *gsi
       break;
 
     case GIMPLE_ASM:
-      /* ??? We ought to come up with a way to add attributes to
-	 asm statements, and then add "transaction_safe" to it.
-	 Either that or get the language spec to resurrect __tm_waiver.  */
       if (d->block_flags & DIAG_TM_SAFE)
 	error_at (gimple_location (stmt),
 		  "asm not allowed in atomic transaction");
@@ -682,7 +680,14 @@  diagnose_tm_1 (gimple_stmt_iterator *gsi
         error_at (gimple_location (stmt),
 		  "asm not allowed in %<transaction_safe%> function");
       else
-	d->saw_unsafe = true;
+	{
+	  /* Mark the ASM as allowed if the function it appears in is
+	     marked as TM pure.  */
+	  if (d->func_flags & DIAG_TM_PURE)
+	    gimple_asm_set_tm_safe (stmt, true);
+	  else
+	    d->saw_unsafe = true;
+	}
       break;
 
     case GIMPLE_TRANSACTION:
@@ -758,6 +763,8 @@  diagnose_tm_blocks (void)
     d.func_flags = DIAG_TM_OUTER | DIAG_TM_SAFE;
   else if (is_tm_safe (current_function_decl))
     d.func_flags = DIAG_TM_SAFE;
+  else if (is_tm_pure (current_function_decl))
+    d.func_flags = DIAG_TM_PURE;
   d.summary_flags = d.func_flags;
 
   memset (&wi, 0, sizeof (wi));
@@ -2322,7 +2329,9 @@  expand_block_tm (struct tm_region *regio
 	  break;
 
 	case GIMPLE_ASM:
-	  gcc_unreachable ();
+	  if (!gimple_asm_tm_safe (stmt))
+	    gcc_unreachable ();
+	  break;
 
 	default:
 	  break;
Index: gimple.h
===================================================================
--- gimple.h	(revision 170359)
+++ gimple.h	(working copy)
@@ -101,6 +101,7 @@  enum gimple_rhs_class
 enum gf_mask {
     GF_ASM_INPUT		= 1 << 0,
     GF_ASM_VOLATILE		= 1 << 1,
+    GF_ASM_TM_SAFE		= 1 << 2,
     GF_CALL_CANNOT_INLINE	= 1 << 0,
     GF_CALL_FROM_THUNK		= 1 << 1,
     GF_CALL_RETURN_SLOT_OPT	= 1 << 2,
@@ -2901,6 +2902,30 @@  gimple_asm_input_p (const_gimple gs)
 }
 
 
+/* If TM_SAFE_P is true, mark asm GS as a TM_ASM_TM_SAFE.
+   This is set for GIMPLE_ASM's that appear in a TM pure function.  */
+
+static inline void
+gimple_asm_set_tm_safe (gimple gs, bool tm_safe_p)
+{
+  GIMPLE_CHECK (gs, GIMPLE_ASM);
+  if (tm_safe_p)
+    gs->gsbase.subcode |= GF_ASM_TM_SAFE;
+  else
+    gs->gsbase.subcode &= ~GF_ASM_TM_SAFE;
+}
+
+
+/* Return true if asm GS is a TM_ASM_TM_SAFE.  */
+
+static inline bool
+gimple_asm_tm_safe (const_gimple gs)
+{
+  GIMPLE_CHECK (gs, GIMPLE_ASM);
+  return (gs->gsbase.subcode & GF_ASM_TM_SAFE) != 0;
+}
+
+
 /* Return the types handled by GIMPLE_CATCH statement GS.  */
 
 static inline tree