diff mbox

do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

Message ID 512BEB2A.9090007@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Feb. 25, 2013, 10:52 p.m. UTC 
Whoops, wrong patch.  This is the latest revision.

On 02/25/13 16:48, Aldy Hernandez wrote:
> On 02/22/13 11:27, Richard Henderson wrote:
>> On 02/21/2013 02:14 PM, Aldy Hernandez wrote:
>>> I suppose we could cheat and avoid passing PR_INSTRUMENTEDCODE if we
>>> ever enter expand_block_tm(), but perhaps we could do a little better as
>>> with the attached patch.
>>
>> I assume you meant "never enter" there.
>>
>> Yes, you don't need to "accumulate" GTMA_INSTRUMENTED_CODE.
>>
>> Probably what should happen is that either
>>
>>>                /* 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;
>>
>> should in fact set EDGE_TM_UNINSTRUMENTED, or we should set that bit
>> even earlier
>
> I think it's best to do this here at tmmark time, instead of at IPA-tm.
>   Don't we have problems when ipa inlining runs after ipa_tm, thus
> creating more instrumented code later on?
>
> If so, the attached patch does it at tmmark.  I also cleaned up the
> instrumented edges, thus generating:
>
>    <bb 2>:
>    tm_state.2_5 = __builtin__ITM_beginTransaction (16458); [
> uninstrumentedCode hasNoAbort doesGoIrrevocable readOnly ]
>    __asm__ __volatile__("");
>    __builtin__ITM_commitTransaction ();
>
>    <bb 3>:
>    return;
>
> Which is way better than what we've ever generated... removing the
> alternative path altogether.  Yay.
>
> How does this look?
> Aldy
>
>>
>>>        /* 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;
>>>          }
>>
>> And while we're at it, ipa_tm_scan_calls_transaction should probably add
>> a check to skip doing ipa_uninstrument_transaction on transactions that
>> have already been so marked.
>
+	* trans-mem.c (execute_tm_mark): Set the region's irrevocable bit
+	if appropriate.
+	(tm_region_init_0): Initialize irrevocable field.
+	(expand_transaction): Remove instrumented edge and blocks if we
+	are sure to go irrevocable.
+	(execute_tm_edges): Skip irrevocable transactions.
+	(execute_tm_memopt): Skip optimization for irrevocable regions.

 2013-02-20  Aldy Hernandez  <aldyh@redhat.com>
 
 	PR middle-end/56108

Comments

Richard Henderson Feb. 26, 2013, 6:24 p.m. UTC | #1 
On 02/25/2013 02:52 PM, Aldy Hernandez wrote:
> I think it's best to do this here at tmmark time, instead of at IPA-tm.
>   Don't we have problems when ipa inlining runs after ipa_tm, thus
> creating more instrumented code later on?

No, I shouldn't think so.  Inlining doesn't change the decision we made during
IPA_TM about whether or not any one transaction doesGoIrr, which is the *only*
bit that's relevant to eliding the uninstrumented code path during IPA_TM, and
thus should be the only bit that's relevant to deciding that the sole code path
is actually supposed to be instrumented or uninstrumented.

I'm not fond of how much extra code and tests this patch is adding.  Is it
really really required?  Is my analysis above wrong in some way?


r~
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/tm/instrumented-mask.c b/gcc/testsuite/gcc.dg/tm/instrumented-mask.c
new file mode 100644
index 0000000..6cfd3e4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tm/instrumented-mask.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -fdump-tree-tmmark" } */
+
+/* If we're sure to go irrevocable, as in the case below, do not pass
+   PR_INSTRUMENTEDCODE to the run-time if there is nothing
+   instrumented within the transaction.  */
+
+int
+main()
+{
+  __transaction_relaxed { __asm__(""); }
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump-times " instrumentedCode" 0 "tmmark" } } */
+/* { dg-final { cleanup-tree-dump "tmmark" } } */
diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c
index 71eaa44..8fe1133 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -1737,6 +1737,11 @@  struct tm_region
 
   /* The set of all blocks that have an TM_IRREVOCABLE call.  */
   bitmap irr_blocks;
+
+  /* True if this transaction has any instrumented code.  False if
+     transaction will immediately go irrevocable upon start.  This bit
+     is set in the tmmark stage, and is not valid before.  */
+  bool has_instrumented_code;
 };
 
 typedef struct tm_region *tm_region_p;
@@ -1785,6 +1790,7 @@  tm_region_init_0 (struct tm_region *outer, basic_block bb, gimple stmt)
 
   region->exit_blocks = BITMAP_ALLOC (&tm_obstack);
   region->irr_blocks = BITMAP_ALLOC (&tm_obstack);
+  region->has_instrumented_code = true;
 
   return region;
 }
@@ -2586,6 +2592,21 @@  expand_transaction (struct tm_region *region, void *data ATTRIBUTE_UNUSED)
         if (e->flags & EDGE_FALLTHRU)
 	  fallthru_edge = e;
       }
+
+    /* If transaction will immediately go irrevocable, thus requiring
+       no instrumented code, get rid of all the instrumentation
+       blocks.  */
+    if (!region->has_instrumented_code)
+      {
+	remove_edge_and_dominated_blocks (inst_edge);
+
+	/* The `inst_edge' was the fallthru edge, and we've just
+	   removed it.  Use the uninst_edge from here on to make
+	   everybody CFG happy.  */
+	uninst_edge->flags |= EDGE_FALLTHRU;
+
+	inst_edge = NULL;
+      }
   }
 
   /* ??? There are plenty of bits here we're not computing.  */
@@ -2631,7 +2652,7 @@  expand_transaction (struct tm_region *region, void *data ATTRIBUTE_UNUSED)
   region->restart_block = region->entry_block;
 
   // Generate log restores.
-  if (!tm_log_save_addresses.is_empty ())
+  if (region->has_instrumented_code && !tm_log_save_addresses.is_empty ())
     {
       basic_block test_bb = create_empty_bb (transaction_bb);
       basic_block code_bb = create_empty_bb (test_bb);
@@ -2679,7 +2700,7 @@  expand_transaction (struct tm_region *region, void *data ATTRIBUTE_UNUSED)
     }
 
   // If we have an ABORT edge, create a test to perform the abort.
-  if (abort_edge)
+  if (region->has_instrumented_code && abort_edge)
     {
       basic_block test_bb = create_empty_bb (transaction_bb);
       if (current_loops && transaction_bb->loop_father)
@@ -2772,7 +2793,8 @@  expand_transaction (struct tm_region *region, void *data ATTRIBUTE_UNUSED)
   // atomic region that shares the first block.  This can cause problems with
   // the transaction restart abnormal edges to be added in the tm_edges pass.
   // Solve this by adding a new empty block to receive the abnormal edges.
-  if (region->restart_block == region->entry_block
+  if (region->has_instrumented_code
+      && region->restart_block == region->entry_block
       && phi_nodes (region->entry_block))
     {
       basic_block empty_bb = create_empty_bb (transaction_bb);
@@ -2871,7 +2893,10 @@  execute_tm_mark (void)
 		 irrevocable right away.  */
 	      if (sub & GTMA_DOES_GO_IRREVOCABLE
 		  && sub & GTMA_MAY_ENTER_IRREVOCABLE)
-		continue;
+		{
+		  r->has_instrumented_code = false;
+		  continue;
+		}
 	    }
 	  expand_block_tm (r, BASIC_BLOCK (i));
 	}
@@ -3047,7 +3072,7 @@  execute_tm_edges (void)
   unsigned i;
 
   FOR_EACH_VEC_ELT (bb_regions, i, r)
-    if (r != NULL)
+    if (r != NULL && r->has_instrumented_code)
       expand_block_edges (r, BASIC_BLOCK (i));
 
   bb_regions.release ();
@@ -3741,6 +3766,9 @@  execute_tm_memopt (void)
       size_t i;
       basic_block bb;
 
+      if (!region->has_instrumented_code)
+	continue;
+
       bitmap_obstack_initialize (&tm_memopt_obstack);
 
       /* Save all BBs for the current region.  */