do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

It has come to my attention, by..ahem...you, that in handling the 
testcase from my previous patch:

   __transaction_relaxed { __asm__(""); }

...if we avoid instrumentation altogether, we shouldn't lie to the 
run-time and pass PR_INSTRUMENTEDCODE.

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.

With this patch, we generate:

   tm_state.2_6 = __builtin__ITM_beginTransaction (16458); [ 
uninstrumentedCode hasNoAbort doesGoIrrevocable readOnly ]

Notice there is no "instrumentedCode".

By the way, can you double check my logic in expand_assign_tm() and 
expand_call_tm(), particularly if we should bother setting 
GTMA_INSTRUMENTED_CODE for thread private variables?

Thanks.

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 3881 bytes --]

commit a441d916db334b71e76d9f4350ffdf992c058b4f
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Thu Feb 21 16:02:11 2013 -0600

    	* trans-mem.c (expand_transaction): Only set PR_INSTRUMENTEDCODE
    	if there was instrumented code.
    	(expand_call_tm): Set GTMA_INSTRUMENTED_CODE.
    	(expand_assign_tm): Same.
    	* gimple.h (GTMA_INSTRUMENTED_CODE): New.

diff --git a/gcc/gimple.h b/gcc/gimple.h
index 4bd6b3d..baa5c24 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -661,6 +661,8 @@ struct GTY(()) gimple_statement_omp_atomic_store {
    tell the runtime that it should begin the transaction in
    serial-irrevocable mode.  */
 #define GTMA_DOES_GO_IRREVOCABLE	(1u << 6)
+/* The transaction has at least one instrumented instruction within.  */
+#define GTMA_INSTRUMENTED_CODE		(1u << 7)
 
 struct GTY(()) gimple_statement_transaction
 {
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..315f00c 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -2146,6 +2146,9 @@ expand_assign_tm (struct tm_region *region, gimple_stmt_iterator *gsi)
     {
       /* Add thread private addresses to log if applicable.  */
       requires_barrier (region->entry_block, lhs, stmt);
+      // ?? Should we set GTMA_INSTRUMENTED_CODE here if there is no
+      // instrumentation, but requires_barrier added a thread private
+      // address to the log?
       gsi_next (gsi);
       return;
     }
@@ -2153,6 +2156,8 @@ expand_assign_tm (struct tm_region *region, gimple_stmt_iterator *gsi)
   // Remove original load/store statement.
   gsi_remove (gsi, true);
 
+  transaction_subcode_ior (region, GTMA_INSTRUMENTED_CODE);
+
   if (load_p && !store_p)
     {
       transaction_subcode_ior (region, GTMA_HAVE_LOAD);
@@ -2230,9 +2235,12 @@ expand_call_tm (struct tm_region *region,
 
   if (fn_decl == builtin_decl_explicit (BUILT_IN_TM_MEMCPY)
       || fn_decl == builtin_decl_explicit (BUILT_IN_TM_MEMMOVE))
-    transaction_subcode_ior (region, GTMA_HAVE_STORE | GTMA_HAVE_LOAD);
+    transaction_subcode_ior (region, GTMA_HAVE_STORE
+			             | GTMA_HAVE_LOAD
+			             | GTMA_INSTRUMENTED_CODE);
   if (fn_decl == builtin_decl_explicit (BUILT_IN_TM_MEMSET))
-    transaction_subcode_ior (region, GTMA_HAVE_STORE);
+    transaction_subcode_ior (region, GTMA_HAVE_STORE
+			             | GTMA_INSTRUMENTED_CODE);
 
   if (is_tm_pure_call (stmt))
     return false;
@@ -2243,7 +2251,8 @@ expand_call_tm (struct tm_region *region,
     {
       /* Assume all non-const/pure calls write to memory, except
 	 transaction ending builtins.  */
-      transaction_subcode_ior (region, GTMA_HAVE_STORE);
+      transaction_subcode_ior (region, GTMA_HAVE_STORE
+			               | GTMA_INSTRUMENTED_CODE);
     }
 
   /* For indirect calls, we already generated a call into the runtime.  */
@@ -2602,7 +2611,7 @@ expand_transaction (struct tm_region *region, void *data ATTRIBUTE_UNUSED)
       flags |= PR_HASNOABORT;
     if ((subcode & GTMA_HAVE_STORE) == 0)
       flags |= PR_READONLY;
-    if (inst_edge)
+    if (inst_edge && (subcode & GTMA_INSTRUMENTED_CODE))
       flags |= PR_INSTRUMENTEDCODE;
     if (uninst_edge)
       flags |= PR_UNINSTRUMENTEDCODE;

Re: do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

[Richard Henderson]

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

>       /* 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.


r~

Re: do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

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.


[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 4645 bytes --]

+	* 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.

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..08f76e2 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 will immediately go irrevocable upon
+     start, thus requiring no instrumented code.  This bit is set in
+     the tmmark stage, and is not valid before.  */
+  bool irrevocable;
 };
 
 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->irrevocable = false;
 
   return region;
 }
@@ -2586,6 +2592,20 @@ expand_transaction (struct tm_region *region, void *data ATTRIBUTE_UNUSED)
         if (e->flags & EDGE_FALLTHRU)
 	  fallthru_edge = e;
       }
+
+    /* If transaction will immediately go irrevocable, get rid of all
+       the instrumentation blocks.  */
+    if (region->irrevocable)
+      {
+	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 +2651,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->irrevocable && !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 +2699,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->irrevocable && abort_edge)
     {
       basic_block test_bb = create_empty_bb (transaction_bb);
       if (current_loops && transaction_bb->loop_father)
@@ -2772,7 +2792,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->irrevocable
+      && region->restart_block == region->entry_block
       && phi_nodes (region->entry_block))
     {
       basic_block empty_bb = create_empty_bb (transaction_bb);
@@ -2871,7 +2892,10 @@ execute_tm_mark (void)
 		 irrevocable right away.  */
 	      if (sub & GTMA_DOES_GO_IRREVOCABLE
 		  && sub & GTMA_MAY_ENTER_IRREVOCABLE)
-		continue;
+		{
+		  r->irrevocable = true;
+		  continue;
+		}
 	    }
 	  expand_block_tm (r, BASIC_BLOCK (i));
 	}
@@ -3047,7 +3071,7 @@ execute_tm_edges (void)
   unsigned i;
 
   FOR_EACH_VEC_ELT (bb_regions, i, r)
-    if (r != NULL)
+    if (r != NULL && !r->irrevocable)
       expand_block_edges (r, BASIC_BLOCK (i));
 
   bb_regions.release ();
@@ -3741,6 +3765,10 @@ execute_tm_memopt (void)
       size_t i;
       basic_block bb;
 
+      // Skip if there will be no instrumented blocks.
+      if (region->irrevocable)
+	continue;
+
       bitmap_obstack_initialize (&tm_memopt_obstack);
 
       /* Save all BBs for the current region.  */

Re: do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

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.
>


[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 4805 bytes --]

+	* 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
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.  */

Re: do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

[Richard Henderson]

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~

Re: do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

On 02/26/13 12:24, Richard Henderson wrote:
> 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.

If inlining doesn't change anything then doing it at IPA time is 
definitely cleaner.  See attached patch.

> 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?

Well, I know you wanted me to avoid calling ipa_uninstrument_transaction 
in ipa_tm_scan_calls_transaction, but the problem is that 
ipa_tm_scan_calls_transaction gets called prior to 
ipa_tm_transform_transaction which is the one setting the GTMA bits.

If you're ok with this revision, I could commit it, and figure something 
out for eliding the call to ipa_uninstrument_transaction as a followup 
patch.  I'm thinking either:

a) Something like the previous patch (which you clearly did not like), 
where we remove the edges ex post facto.

b) Rearrange things somehow so we do ipa_uninstrument_transaction after 
ipa_tm_scan_calls_transaction.

Aldy

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 3660 bytes --]


+	* trans-mem.c (expand_transaction): Do not set PR_INSTRUMENTEDCODE
+	if GTMA_HAS_NO_INSTRUMENTATION.
+	(generate_tm_state): Keep GTMA_HAS_NO_INSTRUMENTATION bit.
+	(ipa_tm_transform_transaction): Set GTMA_HAS_NO_INSTRUMENTATION.
+	* gimple.h (GTMA_HAS_NO_INSTRUMENTATION): Define.
+	* gimple-pretty-print.c (dump_gimple_transaction): Handle
+	GTMA_HAS_NO_INSTRUMENTATION.

diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c
index e7e821d..8c24a57 100644
--- a/gcc/gimple-pretty-print.c
+++ b/gcc/gimple-pretty-print.c
@@ -1399,6 +1399,11 @@ dump_gimple_transaction (pretty_printer *buffer, gimple gs, int spc, int flags)
 		  pp_string (buffer, "GTMA_DOES_GO_IRREVOCABLE ");
 		  subcode &= ~GTMA_DOES_GO_IRREVOCABLE;
 		}
+	      if (subcode & GTMA_HAS_NO_INSTRUMENTATION)
+		{
+		  pp_string (buffer, "GTMA_HAS_NO_INSTRUMENTATION ");
+		  subcode &= ~GTMA_HAS_NO_INSTRUMENTATION;
+		}
 	      if (subcode)
 		pp_printf (buffer, "0x%x ", subcode);
 	      pp_string (buffer, "]");
diff --git a/gcc/gimple.h b/gcc/gimple.h
index 4bd6b3d..1bbd7d7 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -661,6 +661,9 @@ struct GTY(()) gimple_statement_omp_atomic_store {
    tell the runtime that it should begin the transaction in
    serial-irrevocable mode.  */
 #define GTMA_DOES_GO_IRREVOCABLE	(1u << 6)
+/* The transaction contains no instrumentation code whatsover, most
+   likely because it is guaranteed to go irrevocable upon entry.  */
+#define GTMA_HAS_NO_INSTRUMENTATION	(1u << 7)
 
 struct GTY(()) gimple_statement_transaction
 {
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..b0f18b5 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -2602,7 +2602,7 @@ expand_transaction (struct tm_region *region, void *data ATTRIBUTE_UNUSED)
       flags |= PR_HASNOABORT;
     if ((subcode & GTMA_HAVE_STORE) == 0)
       flags |= PR_READONLY;
-    if (inst_edge)
+    if (inst_edge && !(subcode & GTMA_HAS_NO_INSTRUMENTATION))
       flags |= PR_INSTRUMENTEDCODE;
     if (uninst_edge)
       flags |= PR_UNINSTRUMENTEDCODE;
@@ -2806,7 +2806,8 @@ generate_tm_state (struct tm_region *region, void *data ATTRIBUTE_UNUSED)
 
       if (subcode & GTMA_DOES_GO_IRREVOCABLE)
 	subcode &= (GTMA_DECLARATION_MASK | GTMA_DOES_GO_IRREVOCABLE
-		    | GTMA_MAY_ENTER_IRREVOCABLE);
+		    | GTMA_MAY_ENTER_IRREVOCABLE
+		    | GTMA_HAS_NO_INSTRUMENTATION);
       else
 	subcode &= GTMA_DECLARATION_MASK;
       gimple_transaction_set_subcode (region->transaction_stmt, subcode);
@@ -5069,8 +5070,9 @@ ipa_tm_transform_transaction (struct cgraph_node *node)
 	  && 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);
+	  transaction_subcode_ior (region, GTMA_DOES_GO_IRREVOCABLE
+				           | GTMA_MAY_ENTER_IRREVOCABLE
+				   	   | GTMA_HAS_NO_INSTRUMENTATION);
 	  continue;
 	}
 

PING: Re: do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

[Aldy Hernandez]

PING this, or any of my other revisions :)


On 02/27/13 10:43, Aldy Hernandez wrote:
> On 02/26/13 12:24, Richard Henderson wrote:
>> 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.
>
> If inlining doesn't change anything then doing it at IPA time is
> definitely cleaner.  See attached patch.
>
>> 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?
>
> Well, I know you wanted me to avoid calling ipa_uninstrument_transaction
> in ipa_tm_scan_calls_transaction, but the problem is that
> ipa_tm_scan_calls_transaction gets called prior to
> ipa_tm_transform_transaction which is the one setting the GTMA bits.
>
> If you're ok with this revision, I could commit it, and figure something
> out for eliding the call to ipa_uninstrument_transaction as a followup
> patch.  I'm thinking either:
>
> a) Something like the previous patch (which you clearly did not like),
> where we remove the edges ex post facto.
>
> b) Rearrange things somehow so we do ipa_uninstrument_transaction after
> ipa_tm_scan_calls_transaction.
>
> Aldy

Re: do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

[Richard Henderson]

On 02/27/2013 08:43 AM, Aldy Hernandez wrote:
> +	* trans-mem.c (expand_transaction): Do not set PR_INSTRUMENTEDCODE
> +	if GTMA_HAS_NO_INSTRUMENTATION.
> +	(generate_tm_state): Keep GTMA_HAS_NO_INSTRUMENTATION bit.
> +	(ipa_tm_transform_transaction): Set GTMA_HAS_NO_INSTRUMENTATION.
> +	* gimple.h (GTMA_HAS_NO_INSTRUMENTATION): Define.
> +	* gimple-pretty-print.c (dump_gimple_transaction): Handle
> +	GTMA_HAS_NO_INSTRUMENTATION.

This version is ok.


r~