Patchwork [PR,middle-end/56108] handle transactions with ASMs in the first block

login
register
mail settings
Submitter Aldy Hernandez
Date Feb. 20, 2013, 3:35 p.m.
Message ID <5124ED5F.5050302@redhat.com>
Download mbox | patch
Permalink /patch/222105/
State New
Headers show

Comments

Aldy Hernandez - Feb. 20, 2013, 3:35 p.m.
In the following test, the first statement of a relaxed transaction is 
an inline asm:

   __transaction_relaxed { __asm__(""); }

Since we bypass inserting BUILT_IN_TM_IRREVOCABLE at the beginning of 
transactions that are sure to be irrevocable, later when we try to 
expand the transaction, we ICE when we encounter the inline asm.

Currently, we bypass the TM_IRREVOCABLE call here:

  for (region = d->all_tm_regions; region; region = region->next)
     {
       /* If we're sure to go irrevocable, don't transform anything.  */
       if (d->irrevocable_blocks_normal
       && bitmap_bit_p (d->irrevocable_blocks_normal,
                region->entry_block->index))
     {
       transaction_subcode_ior (region, GTMA_DOES_GO_IRREVOCABLE);
       transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE);
       continue;
     }

If I understand this correctly, ideally a transaction marked as 
doesGoIrrevocable shouldn't bother instrumenting the statements inside, 
since the runtime will go irrevocable immediately.  In which case, we 
can elide the instrumentation altogether as the attached patch does.

If my analysis is correct, then testsuite/gcc.dg/tm/memopt-1.c would 
surely go irrevocable, thus requiring no instrumentation, causing the 
memory optimizations to get skipped altogether.  In which case, it's 
best to mark the function calls as safe, so they don't cause the 
transaction to become obviously irrevocable.

Is this correct?  If so, OK?
PR middle-end/56108
        * trans-mem.c (execute_tm_mark): Do not expand transactions that
        are sure to go irrevocable.
    testsuite/
        * gcc.dg/tm/memopt-1.c: Declare functions transaction_safe.
Richard Henderson - Feb. 21, 2013, 5:45 p.m.
On 02/20/2013 07:35 AM, Aldy Hernandez wrote:
> In the following test, the first statement of a relaxed transaction is
> an inline asm:
> 
>   __transaction_relaxed { __asm__(""); }
> 
> Since we bypass inserting BUILT_IN_TM_IRREVOCABLE at the beginning of
> transactions that are sure to be irrevocable, later when we try to
> expand the transaction, we ICE when we encounter the inline asm.
> 
> Currently, we bypass the TM_IRREVOCABLE call here:
> 
>  for (region = d->all_tm_regions; region; region = region->next)
>     {
>       /* If we're sure to go irrevocable, don't transform anything.  */
>       if (d->irrevocable_blocks_normal
>       && bitmap_bit_p (d->irrevocable_blocks_normal,
>                region->entry_block->index))
>     {
>       transaction_subcode_ior (region, GTMA_DOES_GO_IRREVOCABLE);
>       transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE);
>       continue;
>     }
> 
> If I understand this correctly, ideally a transaction marked as
> doesGoIrrevocable shouldn't bother instrumenting the statements inside,
> since the runtime will go irrevocable immediately.  In which case, we
> can elide the instrumentation altogether as the attached patch does.
> 
> If my analysis is correct, then testsuite/gcc.dg/tm/memopt-1.c would
> surely go irrevocable, thus requiring no instrumentation, causing the
> memory optimizations to get skipped altogether.  In which case, it's
> best to mark the function calls as safe, so they don't cause the
> transaction to become obviously irrevocable.
> 
> Is this correct?  If so, OK?

Yes, that's correct.  The patch is ok.


r~

Patch

diff --git a/gcc/testsuite/gcc.dg/tm/memopt-1.c b/gcc/testsuite/gcc.dg/tm/memopt-1.c
index b78a6d4..c5ac5ce 100644
--- a/gcc/testsuite/gcc.dg/tm/memopt-1.c
+++ b/gcc/testsuite/gcc.dg/tm/memopt-1.c
@@ -2,8 +2,8 @@ 
 /* { dg-options "-fgnu-tm -O -fdump-tree-tmmemopt" } */
 
 long g, xxx, yyy;
-extern george() __attribute__((transaction_callable));
-extern ringo(long int);
+extern george() __attribute__((transaction_safe));
+extern ringo(long int) __attribute__((transaction_safe));
 int i;
 
 f()
diff --git a/gcc/testsuite/gcc.dg/tm/pr56108.c b/gcc/testsuite/gcc.dg/tm/pr56108.c
new file mode 100644
index 0000000..81ff574
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tm/pr56108.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -O" } */
+
+int
+main()
+{
+  __transaction_relaxed { __asm__(""); }
+  return 0;
+}
diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c
index dd3918e..71eaa44 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -2859,8 +2859,23 @@  execute_tm_mark (void)
   // Expand memory operations into calls into the runtime.
   // This collects log entries as well.
   FOR_EACH_VEC_ELT (bb_regions, i, r)
-    if (r != NULL)
-      expand_block_tm (r, BASIC_BLOCK (i));
+    {
+      if (r != NULL)
+	{
+	  if (r->transaction_stmt)
+	    {
+	      unsigned sub = gimple_transaction_subcode (r->transaction_stmt);
+
+	      /* If we're sure to go irrevocable, there won't be
+		 anything to expand, since the run-time will go
+		 irrevocable right away.  */
+	      if (sub & GTMA_DOES_GO_IRREVOCABLE
+		  && sub & GTMA_MAY_ENTER_IRREVOCABLE)
+		continue;
+	    }
+	  expand_block_tm (r, BASIC_BLOCK (i));
+	}
+    }
 
   bb_regions.release ();