| Message ID | 1319792945.5756.899.camel@triegel.csb |
|---|---|
| State | New |
| Headers | show |
> 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
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.
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
--- 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; }
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? 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.