diff mbox

do not pass PR_INSTRUMENTEDCODE if there is no instrumentation

Message ID 512E379E.2060906@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Feb. 27, 2013, 4:43 p.m. UTC 
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
+	* 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.

Comments

Aldy Hernandez March 7, 2013, 5:25 p.m. UTC | #1 
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
Richard Henderson March 8, 2013, 8:10 p.m. UTC | #2 
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~
diff mbox

Patch

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;
 	}