[trans-mem] Explicitly go irrevocable even if transaction will always go irrevocable.

[trans-mem] Explicitly go irrevocable even if transaction will always go irrevocable.

[Torvald Riegel]

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

The ABI does not require a TM runtime library to immediately start a
transaction in irrevocable mode if the beginTransaction flag
"doesGoIrrevocable" is set. This patch fixes this by inserting a call to
changeTransactionMode(0) into the entry block of all transactions that
always go irrevocable eventually.
Once we generate uninstrumented code paths too, we can optimize this by
telling the runtime that it is probably preferable to use uninstrumented
code right away. The instrumented path, if we generate it, should then
go irrevocable as late as possible, so that there is actually a choice
that the TM runtime can make (e.g., if we go irrevocable close to the
end of a long transaction, then being irrevocable for a shorter time can
increase overall performance).

OK for branch?

[-- Attachment #2: patch2 --]
[-- Type: text/plain, Size: 2977 bytes --]

commit 64e5f9da61ea7f45748b411aa58404628040c343
Author: Torvald Riegel <triegel@redhat.com>
Date:   Fri Oct 28 10:55:38 2011 +0200

    Explicitly go irrevocable even if transaction will always go irrevocable.
    
    	* trans-mem.c (ipa_tm_transform_transaction): Insert explicit request
    	to go irrevocable even if transaction will always go irrevocable.
    	* testsuite/gcc.dg/tm/irrevocable-8.c: New file.
    	* testsuite/gcc.dg/tm/memopt-1.c: Re-focus this test on tmmemopt.

--- a/gcc/ChangeLog.tm
+++ b/gcc/ChangeLog.tm
@@ -1,5 +1,12 @@
 2011-10-27  Torvald Riegel  <triegel@redhat.com>
 
+	* trans-mem.c (ipa_tm_transform_transaction): Insert explicit request
+	to go irrevocable even if transaction will always go irrevocable.
+	* testsuite/gcc.dg/tm/irrevocable-8.c: New file.
+	* testsuite/gcc.dg/tm/memopt-1.c: Re-focus this test on tmmemopt.
+
+2011-10-27  Torvald Riegel  <triegel@redhat.com>
+
 	* trans-mem.c (struct tm_region): Extended comment.
 	(tm_region_init): Fix tm_region association of blocks with the "over"
 	label used for transcation abort.
diff --git a/gcc/testsuite/gcc.dg/tm/irrevocable-8.c b/gcc/testsuite/gcc.dg/tm/irrevocable-8.c
new file mode 100644
index 0000000..1fe6c0a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tm/irrevocable-8.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -fdump-ipa-tmipa" } */
+
+void unsafe(void) __attribute__((transaction_unsafe));
+
+void
+f(void)
+{
+  __transaction_relaxed {
+    unsafe();
+  }
+}
+
+/* { dg-final { scan-ipa-dump-times "changeTransactionMode \\(0\\)" 1 "tmipa" } } */
+/* { dg-final { cleanup-ipa-dump "tmipa" } } */
diff --git a/gcc/testsuite/gcc.dg/tm/memopt-1.c b/gcc/testsuite/gcc.dg/tm/memopt-1.c
index 06d4f64..9a48dcb 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/trans-mem.c b/gcc/trans-mem.c
index e88c7ad..994cf09 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -4521,6 +4521,14 @@ ipa_tm_transform_transaction (struct cgraph_node *node)
 	{
 	  transaction_subcode_ior (region, GTMA_DOES_GO_IRREVOCABLE);
 	  transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE);
+	  /* ??? We still have to insert a call to explicitly request
+	     to go irrevocable because the ABI does not require the runtime
+	     to immediately go irrevocable.  Once we generate an
+	     uninstrumented code path for transactions, we can avoid this
+	     extra call and only make uninstrumented code available, which
+	     tells the runtime that it must go irrevocable immediately.  */
+	  ipa_tm_insert_irr_call (node, region, region->entry_block);
+	  need_ssa_rename = true;
 	  continue;
 	}
 

Re: [trans-mem] Explicitly go irrevocable even if transaction will always go irrevocable.

[Aldy Hernandez]

> diff --git a/gcc/testsuite/gcc.dg/tm/memopt-1.c b/gcc/testsuite/gcc.dg/tm/memopt-1.c
> index 06d4f64..9a48dcb 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;

The patch looks fine, but...

Was the original test wrong, or are you testing something new?

If the original test was wrong, this patch is OK.

If the original test was not wrong, you need to add a new test (and 
bonus points for finding out why this test is currently failing :)).

Thanks.
Aldy

Re: [trans-mem] Explicitly go irrevocable even if transaction will always go irrevocable.

[Patrick Marlier]

On 10/28/2011 08:53 AM, Aldy Hernandez wrote:
> If the original test was not wrong, you need to add a new test (and
> bonus points for finding out why this test is currently failing :)).

long g, xxx, yyy;

/* { dg-final { scan-tree-dump-times "transforming: .*_ITM_RaWU8 
\\(&g\\);" 1 "tmmemopt" } } */

At least, maybe one of the problem is that g is "long" type (4 bytes on 
32bits) and it is testing for 8 bytes?

Patrick.

Re: [trans-mem] Explicitly go irrevocable even if transaction will always go irrevocable.

[Torvald Riegel]

On Fri, 2011-10-28 at 07:53 -0500, Aldy Hernandez wrote:
> > diff --git a/gcc/testsuite/gcc.dg/tm/memopt-1.c b/gcc/testsuite/gcc.dg/tm/memopt-1.c
> > index 06d4f64..9a48dcb 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;
> 
> The patch looks fine, but...

Looking closer at this, we were faking to have an uninstrumented code
path so not explicitly requesting irrevocable mode was okay. However, we
still had calls to the TM library in those code, which is not really
what we want (mostly for performance reasons, it is supposed to still
work because the runtime has to change to a suitable dispatch internally
after going irrevocable).
I'll prepare a different patch, after looking at Richard's recent
changes.

> Was the original test wrong, or are you testing something new?

Yes, sort of. It was testing for optimizations that were correct
performed but which should not have been applicable in this particular
test case _and_ with the current state of the code. However, after the
fix for when to request irrevocable mode that I have in mind, this test
should work as is.

Torvald