On 02/02/2012 04:22 AM, Richard Guenther wrote:
On Wed, Feb 1, 2012 at 10:19 PM, Patrick Marlier
<patrick.marl...@gmail.com> wrote:
On 02/01/2012 03:59 AM, Richard Guenther wrote:
The patch looks ok, but I'm not sure why you do not get a cgraph node
here - cgraph doesn't really care for builtins as far as I can see.
Honza?
I cannot help on this...
Don't you maybe want to handle builtins in a special way here?
Indeed, I think my patch is wrong. __builtin_prefetch should have the
transaction_pure attribute. I don't know how usually it should be done but
what about adding a gcc_assert before to dereference node (potentially
NULL)?
How the attached patch looks like now?
(Tested on i686)
At least those that are const/pure?
About const/pure, we cannot consider those functions as transaction_pure
because they can read global and shared variable.
Well, const functions cannot access global memory, they can only inspect
their arguments.
Actually, in this example, GCC does not complain at all about those
const functions which read global memory... Should it? or the user is
allowed to indicate const even if it is not?
static int a = 0;
int f (int* a) __attribute__((const));
int f (int* a)
{
return *a+1;
}
int g () __attribute__((const));
int g ()
{
return a+1;
}
int main()
{
int tmp = 0;
__transaction_atomic {
tmp += f(&a);
tmp += g();
}
return tmp;
}
In this test, the transaction is completely removed since no
transactional operation is detected.
Of course __builtin_prefetch seems to be special in some way. Note that
users can explicitely call it, so setting the attribute from the prefetching
pass isn't the correct thing to do.
If the user calls it explicitly, at least the compiler doesn't ICE.
Ex: error: unsafe function call ‘__builtin_prefetch’ within atomic
transaction.
So what's your proposal? to add directly in builtins.def the
transaction_pure attribute to required functions?
Note that __builtin_prefetch has the 'no vops' attribute - I think you should
simply consider all 'no vops' builtins as transaction pure instead or
explicitely
consider a set of builtins as transaction pure (that's more scalable than
sticking the attribute onto random builtins - see how we handle builtins in
the alias machinery, we avoid sticking the fnspec attribute onto each
builtin but simply have special handling for them).
'no vops' attribute description is quite vague. "may have arbitrary side
effects" scares me ;)
I will have a look at this alias machinery (any files particularly?
tree.c seems to have a lot of builtins things in it).
In this patch, I do manage 'no vops' the same way as const is.
Attached the new version where we add ECF_TM_PURE to NOVOPS functions.
Tested on x86_64-unknown-linux-gnu.
Thanks!
Patrick.
2012-02-02 Patrick Marlier <patrick.marl...@gmail.com>
PR middle-end/52047
* trans-mem.c (expand_call_tm): Add an assertion.
* calls.c (flags_from_decl_or_type): Add ECF_TM_PURE to 'no vops'
functions.
Thanks,
Richard.
BTW, I will post a PR (and probably a patch) about this.
Thanks for your comment!
Patrick.
PR middle-end/52047
* trans-mem.c (expand_call_tm): Add an assertion.
* tree-ssa-loop-prefetch.c (issue_prefetch_ref): Add transaction_pure
attribute to __builtin_prefetch.
(tree_ssa_prefetch_arrays): Likewise.
Index: calls.c
===================================================================
--- calls.c (revision 183837)
+++ calls.c (working copy)
@@ -716,7 +716,7 @@ flags_from_decl_or_type (const_tree exp)
{
if (is_tm_builtin (exp))
flags |= ECF_TM_BUILTIN;
- else if ((flags & ECF_CONST) != 0
+ else if ((flags & (ECF_CONST|ECF_NOVOPS)) != 0
|| lookup_attribute ("transaction_pure",
TYPE_ATTRIBUTES (TREE_TYPE (exp))))
flags |= ECF_TM_PURE;
Index: trans-mem.c
===================================================================
--- trans-mem.c (revision 183837)
+++ trans-mem.c (working copy)
@@ -1,5 +1,5 @@
/* Passes for transactional memory support.
- Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+ Copyright (C) 2008, 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
This file is part of GCC.
@@ -2267,6 +2267,8 @@ expand_call_tm (struct tm_region *region,
}
node = cgraph_get_node (fn_decl);
+ /* All calls should have cgraph here. */
+ gcc_assert (node);
if (node->local.tm_may_enter_irr)
transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE);
Index: testsuite/gcc.dg/tm/pr52047.c
===================================================================
--- testsuite/gcc.dg/tm/pr52047.c (revision 0)
+++ testsuite/gcc.dg/tm/pr52047.c (revision 0)
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fgnu-tm -fprefetch-loop-arrays -w" } */
+
+int test2 (int x[])
+{
+ return x[12];
+}
+
+int test1 (void)
+{
+ int x[1000], i;
+ for (i = 0; i < 1000; i++)
+ x[i] = i;
+ return test2 (x);
+}
+
+int
+main ()
+{
+ __transaction_atomic
+ {
+ if (test1 ())
+ __transaction_cancel;
+ }
+ return 0;
+}
