* Re: [trans-mem] issue with openmp
[not found] <4C04C24A.4080103@unine.ch>
@ 2010-06-18 16:47 ` Aldy Hernandez
2010-06-22 18:30 ` Aldy Hernandez
[not found] ` <20100618153140.GB7343@redhat.com>
1 sibling, 1 reply; 9+ messages in thread
From: Aldy Hernandez @ 2010-06-18 16:47 UTC (permalink / raw)
To: Patrick Marlier
Cc: Richard Henderson, FELBER Pascal, Javier Arias, gcc-patches
The problem here is that gimplify_transaction() places the temporaries
that were generated for a transaction in cfun->local_decls, but
omp_copy_decl() will only look in the enclosing contexts, not in
cfun->local_decls.
rth suggested we make a better attempt at putting temporaries into the
proper context so OMP can figure out how to pull pieces out to make a
new function.
The patch below wraps the transaction bodies into a BIND_EXPR, which
gimplify_transaction() can later use for its temporaries, thus allowing
the OMP code to find a proper context.
OK for branch?
* c-typeck.c (c_finish_transaction): Same.
* cp/semantics.c (finish_transaction_stmt): Wrap transaction body
in a BIND_EXPR.
Index: testsuite/c-c++-common/tm/omp.c
===================================================================
--- testsuite/c-c++-common/tm/omp.c (revision 0)
+++ testsuite/c-c++-common/tm/omp.c (revision 0)
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -fopenmp" } */
+
+__attribute__ ((transaction_pure))
+unsigned long rdtsc();
+
+typedef struct ENTER_EXIT_TIMES
+{
+ unsigned long enter;
+} times_t;
+
+void ParClassify()
+{
+ void * Parent;
+#pragma omp parallel private(Parent)
+ {
+ times_t inside;
+ __transaction [[atomic]] {
+ inside.enter = rdtsc();
+ }
+ }
+}
Index: cp/semantics.c
===================================================================
--- cp/semantics.c (revision 160538)
+++ cp/semantics.c (working copy)
@@ -4683,7 +4683,18 @@ begin_transaction_stmt (location_t loc,
void
finish_transaction_stmt (tree stmt, tree compound_stmt, int flags)
{
- TRANSACTION_EXPR_BODY (stmt) = pop_stmt_list (TRANSACTION_EXPR_BODY (stmt));
+ tree body = pop_stmt_list (TRANSACTION_EXPR_BODY (stmt));
+
+ /* Wrap the transaction body in a BIND_EXPR so we have a context
+ where to put decls for OpenMP. */
+ if (TREE_CODE (body) != BIND_EXPR)
+ {
+ body = build3 (BIND_EXPR, void_type_node, NULL, body, NULL);
+ TREE_SIDE_EFFECTS (body) = 1;
+ SET_EXPR_LOCATION (body, EXPR_LOCATION (stmt));
+ }
+
+ TRANSACTION_EXPR_BODY (stmt) = body;
TRANSACTION_EXPR_OUTER (stmt) = (flags & TM_STMT_ATTR_OUTER) != 0;
TRANSACTION_EXPR_RELAXED (stmt) = (flags & TM_STMT_ATTR_RELAXED) != 0;
Index: c-typeck.c
===================================================================
--- c-typeck.c (revision 160538)
+++ c-typeck.c (working copy)
@@ -10281,9 +10281,20 @@ c_finish_omp_clauses (tree clauses)
/* Create a transaction node. */
tree
-c_finish_transaction (location_t loc, tree block, int flags)
+c_finish_transaction (location_t loc, tree body, int flags)
{
- tree stmt = build_stmt (loc, TRANSACTION_EXPR, block);
+ tree stmt;
+
+ /* Wrap the transaction body in a BIND_EXPR so we have a context
+ where to put decls for OpenMP. */
+ if (TREE_CODE (body) != BIND_EXPR)
+ {
+ body = build3 (BIND_EXPR, void_type_node, NULL, body, NULL);
+ TREE_SIDE_EFFECTS (body) = 1;
+ SET_EXPR_LOCATION (body, loc);
+ }
+
+ stmt = build_stmt (loc, TRANSACTION_EXPR, body);
if (flags & TM_STMT_ATTR_OUTER)
TRANSACTION_EXPR_OUTER (stmt) = 1;
if (flags & TM_STMT_ATTR_RELAXED)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [trans-mem] issue with openmp
2010-06-18 16:47 ` [trans-mem] issue with openmp Aldy Hernandez
@ 2010-06-22 18:30 ` Aldy Hernandez
2010-06-22 19:00 ` Richard Henderson
0 siblings, 1 reply; 9+ messages in thread
From: Aldy Hernandez @ 2010-06-22 18:30 UTC (permalink / raw)
To: Patrick Marlier
Cc: Richard Henderson, FELBER Pascal, Javier Arias, gcc-patches
On Fri, Jun 18, 2010 at 11:26:00AM -0400, Aldy Hernandez wrote:
> The problem here is that gimplify_transaction() places the temporaries
> that were generated for a transaction in cfun->local_decls, but
> omp_copy_decl() will only look in the enclosing contexts, not in
> cfun->local_decls.
>
> rth suggested we make a better attempt at putting temporaries into the
> proper context so OMP can figure out how to pull pieces out to make a
> new function.
>
> The patch below wraps the transaction bodies into a BIND_EXPR, which
> gimplify_transaction() can later use for its temporaries, thus allowing
> the OMP code to find a proper context.
>
> OK for branch?
Meanwhile, back at the ranch... rth complains that we should do this in
the gimplifier and save the front-end work.
Yay, less code!
OK for branch?
* gimplify.c (gimplify_transaction): Wrap transaction body
in a BIND_EXPR.
Index: testsuite/c-c++-common/tm/omp.c
===================================================================
--- testsuite/c-c++-common/tm/omp.c (revision 0)
+++ testsuite/c-c++-common/tm/omp.c (revision 0)
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -fopenmp" } */
+
+__attribute__ ((transaction_pure))
+unsigned long rdtsc();
+
+typedef struct ENTER_EXIT_TIMES
+{
+ unsigned long enter;
+} times_t;
+
+void ParClassify()
+{
+ void * Parent;
+#pragma omp parallel private(Parent)
+ {
+ times_t inside;
+ __transaction [[atomic]] {
+ inside.enter = rdtsc();
+ }
+ }
+}
Index: gimplify.c
===================================================================
--- gimplify.c (revision 161187)
+++ gimplify.c (working copy)
@@ -6385,20 +6385,27 @@ gimplify_omp_atomic (tree *expr_p, gimpl
static enum gimplify_status
gimplify_transaction (tree *expr_p, gimple_seq *pre_p)
{
- tree expr = *expr_p, temp;
+ tree expr = *expr_p, temp, tbody = TRANSACTION_EXPR_BODY (expr);
gimple g;
gimple_seq body = NULL;
struct gimplify_ctx gctx;
int subcode = 0;
+ /* Wrap the transaction body in a BIND_EXPR so we have a context
+ where to put decls for OpenMP. */
+ if (TREE_CODE (tbody) != BIND_EXPR)
+ {
+ tree bind = build3 (BIND_EXPR, void_type_node, NULL, tbody, NULL);
+ TREE_SIDE_EFFECTS (bind) = 1;
+ SET_EXPR_LOCATION (bind, EXPR_LOCATION (tbody));
+ TRANSACTION_EXPR_BODY (expr) = bind;
+ }
+
push_gimplify_context (&gctx);
temp = voidify_wrapper_expr (*expr_p, NULL);
g = gimplify_and_return_first (TRANSACTION_EXPR_BODY (expr), &body);
- if (g && gimple_code (g) == GIMPLE_BIND)
- pop_gimplify_context (g);
- else
- pop_gimplify_context (NULL);
+ pop_gimplify_context (g);
g = gimple_build_transaction (body, NULL);
if (TRANSACTION_EXPR_OUTER (expr))
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [trans-mem] issue with openmp
2010-06-22 18:30 ` Aldy Hernandez
@ 2010-06-22 19:00 ` Richard Henderson
0 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2010-06-22 19:00 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: Patrick Marlier, FELBER Pascal, Javier Arias, gcc-patches
On 06/22/2010 11:17 AM, Aldy Hernandez wrote:
> On Fri, Jun 18, 2010 at 11:26:00AM -0400, Aldy Hernandez wrote:
>> The problem here is that gimplify_transaction() places the temporaries
>> that were generated for a transaction in cfun->local_decls, but
>> omp_copy_decl() will only look in the enclosing contexts, not in
>> cfun->local_decls.
>>
>> rth suggested we make a better attempt at putting temporaries into the
>> proper context so OMP can figure out how to pull pieces out to make a
>> new function.
>>
>> The patch below wraps the transaction bodies into a BIND_EXPR, which
>> gimplify_transaction() can later use for its temporaries, thus allowing
>> the OMP code to find a proper context.
>>
>> OK for branch?
>
> Meanwhile, back at the ranch... rth complains that we should do this in
> the gimplifier and save the front-end work.
>
> Yay, less code!
>
> OK for branch?
>
> * gimplify.c (gimplify_transaction): Wrap transaction body
> in a BIND_EXPR.
Ok.
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [trans-mem] issue with openmp
[not found] ` <4C225BC3.9010301@unine.ch>
@ 2010-07-06 17:35 ` Aldy Hernandez
2010-07-06 17:48 ` Richard Henderson
0 siblings, 1 reply; 9+ messages in thread
From: Aldy Hernandez @ 2010-07-06 17:35 UTC (permalink / raw)
To: Patrick Marlier; +Cc: Richard Henderson, FELBER Pascal, gcc-patches
> >>- libitm : I think it is useful to add _ITM_malloc, _ITM_free,
> >>_ITM_calloc into libitm.h and also to add ITM_REGPARM because I had
> >>problems when I wanted to use directly the library.
Patrick found a problem while calling the _ITM_malloc/etc functions
directly (instead of through the wrappers). I've added prototypes for
them.
OK for branch?
Patrick, please verify that this fixes any problems on your end.
* libitm.h (ITM_PURE): Define.
Declare _ITM_malloc, _ITM_calloc, and _ITM_free.
Index: libitm.h
===================================================================
--- libitm.h (revision 161512)
+++ libitm.h (working copy)
@@ -43,6 +43,7 @@ extern "C" {
#endif
#define ITM_NORETURN __attribute__((noreturn))
+#define ITM_PURE __attribute__((transaction_pure))
\f
/* The following are externally visible definitions and functions, though
only very few of these should be called by user code. */
@@ -152,8 +153,13 @@ extern void _ITM_addUserUndoAction(_ITM_
extern int _ITM_getThreadnum(void) ITM_REGPARM;
-__attribute__((transaction_pure))
-extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM;
+extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM ITM_PURE;
+
+__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
+extern void *_ITM_malloc (size_t);
+__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
+extern void *_ITM_calloc (size_t, size_t);
+extern void _ITM_free (void *) ITM_REGPARM ITM_PURE;
/* The following typedefs exist to make the macro expansions below work
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [trans-mem] issue with openmp
2010-07-06 17:35 ` Aldy Hernandez
@ 2010-07-06 17:48 ` Richard Henderson
2010-07-06 18:12 ` Aldy Hernandez
0 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2010-07-06 17:48 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: Patrick Marlier, FELBER Pascal, gcc-patches
On 07/06/2010 09:29 AM, Aldy Hernandez wrote:
> -__attribute__((transaction_pure))
> -extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM;
> +extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM ITM_PURE;
> +
> +__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
> +extern void *_ITM_malloc (size_t);
> +__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
> +extern void *_ITM_calloc (size_t, size_t);
> +extern void _ITM_free (void *) ITM_REGPARM ITM_PURE;
I'm not sure that _ITM_malloc et al should include ITM_REGPARM.
Also, it's canonical to place these attributes before the ";",
not before the "extern".
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [trans-mem] issue with openmp
2010-07-06 17:48 ` Richard Henderson
@ 2010-07-06 18:12 ` Aldy Hernandez
2010-07-06 19:28 ` Richard Henderson
0 siblings, 1 reply; 9+ messages in thread
From: Aldy Hernandez @ 2010-07-06 18:12 UTC (permalink / raw)
To: Richard Henderson; +Cc: Patrick Marlier, FELBER Pascal, gcc-patches
On Tue, Jul 06, 2010 at 10:48:20AM -0700, Richard Henderson wrote:
> On 07/06/2010 09:29 AM, Aldy Hernandez wrote:
> > -__attribute__((transaction_pure))
> > -extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM;
> > +extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM ITM_PURE;
> > +
> > +__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
> > +extern void *_ITM_malloc (size_t);
> > +__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
> > +extern void *_ITM_calloc (size_t, size_t);
> > +extern void _ITM_free (void *) ITM_REGPARM ITM_PURE;
>
> I'm not sure that _ITM_malloc et al should include ITM_REGPARM.
Arghh, I was getting confused by a complaint by Patrick about a
%eax<->%rax problem in the calling sequence, but that must be something
different because I can't reproduce it. Direct calls to _ITM_malloc()
agree with the calling convention expected by such function.
Patrick, if after a complete toolchain rebuild with this patch, you
still see an ABI problem, send me the testcase.
> Also, it's canonical to place these attributes before the ";",
> not before the "extern".
Fixed.
OK?
* libitm.h (ITM_PURE): Define.
Declare _ITM_malloc, _ITM_calloc, and _ITM_free.
Index: libitm.h
===================================================================
--- libitm.h (revision 161512)
+++ libitm.h (working copy)
@@ -43,6 +43,7 @@ extern "C" {
#endif
#define ITM_NORETURN __attribute__((noreturn))
+#define ITM_PURE __attribute__((transaction_pure))
\f
/* The following are externally visible definitions and functions, though
only very few of these should be called by user code. */
@@ -152,8 +153,15 @@ extern void _ITM_addUserUndoAction(_ITM_
extern int _ITM_getThreadnum(void) ITM_REGPARM;
-__attribute__((transaction_pure))
-extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM;
+extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM ITM_PURE;
+
+extern void *_ITM_malloc (size_t)
+ __attribute__((__malloc__)) ITM_PURE;
+
+extern void *_ITM_calloc (size_t, size_t)
+ __attribute__((__malloc__)) ITM_PURE;
+
+extern void _ITM_free (void *) ITM_PURE;
/* The following typedefs exist to make the macro expansions below work
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [trans-mem] issue with openmp
2010-07-06 18:12 ` Aldy Hernandez
@ 2010-07-06 19:28 ` Richard Henderson
2010-07-06 20:58 ` Patrick Marlier
0 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2010-07-06 19:28 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: Patrick Marlier, FELBER Pascal, gcc-patches
On 07/06/2010 11:12 AM, Aldy Hernandez wrote:
> On Tue, Jul 06, 2010 at 10:48:20AM -0700, Richard Henderson wrote:
>> On 07/06/2010 09:29 AM, Aldy Hernandez wrote:
>>> -__attribute__((transaction_pure))
>>> -extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM;
>>> +extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM ITM_PURE;
>>> +
>>> +__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
>>> +extern void *_ITM_malloc (size_t);
>>> +__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
>>> +extern void *_ITM_calloc (size_t, size_t);
>>> +extern void _ITM_free (void *) ITM_REGPARM ITM_PURE;
>>
>> I'm not sure that _ITM_malloc et al should include ITM_REGPARM.
>
> Arghh, I was getting confused by a complaint by Patrick about a
> %eax<->%rax problem in the calling sequence, but that must be something
> different because I can't reproduce it.
That was due to C's implicit type of "int" for undeclared functions.
> * libitm.h (ITM_PURE): Define.
> Declare _ITM_malloc, _ITM_calloc, and _ITM_free.
Ok.
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [trans-mem] issue with openmp
2010-07-06 19:28 ` Richard Henderson
@ 2010-07-06 20:58 ` Patrick Marlier
2010-07-06 23:26 ` Richard Henderson
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Marlier @ 2010-07-06 20:58 UTC (permalink / raw)
To: Richard Henderson; +Cc: Aldy Hernandez, FELBER Pascal, gcc-patches
On 07/06/2010 09:28 PM, Richard Henderson wrote:
> On 07/06/2010 11:12 AM, Aldy Hernandez wrote:
>> On Tue, Jul 06, 2010 at 10:48:20AM -0700, Richard Henderson wrote:
>>> On 07/06/2010 09:29 AM, Aldy Hernandez wrote:
>>>> -__attribute__((transaction_pure))
>>>> -extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM;
>>>> +extern void _ITM_dropReferences (void *, size_t) ITM_REGPARM ITM_PURE;
>>>> +
>>>> +__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
>>>> +extern void *_ITM_malloc (size_t);
>>>> +__attribute__((__malloc__)) ITM_REGPARM ITM_PURE
>>>> +extern void *_ITM_calloc (size_t, size_t);
>>>> +extern void _ITM_free (void *) ITM_REGPARM ITM_PURE;
>>>
>>> I'm not sure that _ITM_malloc et al should include ITM_REGPARM.
Can you explain me why _ITM_malloc shouldn't have ITM_REGPARM whereas
for example _ITM_memsetW must have it?
>> Arghh, I was getting confused by a complaint by Patrick about a
>> %eax<->%rax problem in the calling sequence, but that must be something
>> different because I can't reproduce it.
>
> That was due to C's implicit type of "int" for undeclared functions.
>
>> * libitm.h (ITM_PURE): Define.
>> Declare _ITM_malloc, _ITM_calloc, and _ITM_free.
Yes it solved my issue.
Thank you!
Patrick.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [trans-mem] issue with openmp
2010-07-06 20:58 ` Patrick Marlier
@ 2010-07-06 23:26 ` Richard Henderson
0 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2010-07-06 23:26 UTC (permalink / raw)
To: Patrick Marlier; +Cc: Aldy Hernandez, FELBER Pascal, gcc-patches
On 07/06/2010 01:58 PM, Patrick Marlier wrote:
> Can you explain me why _ITM_malloc shouldn't have ITM_REGPARM whereas
> for example _ITM_memsetW must have it?
The other functions in that header are defined as part of the Intel ABI,
and that ABI specifies REGPARM. _ITM_malloc is not part of that ABI.
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-07-06 23:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <4C04C24A.4080103@unine.ch>
2010-06-18 16:47 ` [trans-mem] issue with openmp Aldy Hernandez
2010-06-22 18:30 ` Aldy Hernandez
2010-06-22 19:00 ` Richard Henderson
[not found] ` <20100618153140.GB7343@redhat.com>
[not found] ` <4C221F08.2020403@unine.ch>
[not found] ` <20100623172043.GB19259@redhat.com>
[not found] ` <4C225BC3.9010301@unine.ch>
2010-07-06 17:35 ` Aldy Hernandez
2010-07-06 17:48 ` Richard Henderson
2010-07-06 18:12 ` Aldy Hernandez
2010-07-06 19:28 ` Richard Henderson
2010-07-06 20:58 ` Patrick Marlier
2010-07-06 23:26 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).