| Message ID | 4D46FD58.5050207@redhat.com |
|---|---|
| State | New |
| Headers | show |
In function expand_call_tm(), the _ITM_memsetW must indicate that the
transaction is a not a read only transaction (not optional).
Here kind of test for testsuite.
/* { dg-do compile } */
/* { dg-options "-fgnu-tm -fdump-tree-tmedge -fdump-tree-tmlower" } */
char array[4] = "aaaa";
int main()
{
__transaction [[atomic]] {
memset(array, 'b', sizeof(4));
}
}
/* { dg-final { scan-tree-dump-times "readOnly" 0 "tmedge" } } */
/* { dg-final { scan-tree-dump-times "GTMA_HAVE_STORE" 1 "tmlower" } } */
/* { dg-final { cleanup-tree-dump "tmedge" } } */
/* { dg-final { cleanup-tree-dump "tmlower" } } */
This transaction must not be read only.
The part of my draft patch about this:
+ fn_decl = gimple_call_fndecl (stmt);
+ /* ??? Do the same for ITM memcpy/memmove */
+ if (is_tm_memset (fn_decl))
+ transaction_subcode_ior (region, GTMA_HAVE_STORE);
Thanks.
Patrick.
On Mon, 31 Jan 2011, Aldy Hernandez wrote:
> Transform memset into its transactional equivalent.
>
> Thanks to Patrick for doing all the dirty work on this patch.
>
> OK for branch?
>
On Monday 31 January 2011 23:17:52 Patrick Marlier wrote: > The part of my draft patch about this: > + fn_decl = gimple_call_fndecl (stmt); > + /* ??? Do the same for ITM memcpy/memmove */ > + if (is_tm_memset (fn_decl)) > + transaction_subcode_ior (region, GTMA_HAVE_STORE); Any wrapper can potentially execute a store. If this is not supposed to happen, this should be a added as a requirement in the ABI. Torvald
On 01/31/11 16:17, Patrick Marlier wrote: > In function expand_call_tm(), the _ITM_memsetW must indicate that the > transaction is a not a read only transaction (not optional). ... > This transaction must not be read only. > > The part of my draft patch about this: > + fn_decl = gimple_call_fndecl (stmt); > + /* ??? Do the same for ITM memcpy/memmove */ > + if (is_tm_memset (fn_decl)) > + transaction_subcode_ior (region, GTMA_HAVE_STORE); Actually, if you look a few lines down, you will notice that we always set GTMA_HAVE_{STORE,LOAD} for non TM pure calls: /* Note that something may happen. */ *state |= GTMA_HAVE_LOAD | GTMA_HAVE_STORE; So the transaction will never be marked as read-only incorrectly. Richard, is the patch OK?
On 02/02/2011 04:41 PM, Aldy Hernandez wrote: > On 01/31/11 16:17, Patrick Marlier wrote: >> In function expand_call_tm(), the _ITM_memsetW must indicate that the >> transaction is a not a read only transaction (not optional). > ... >> This transaction must not be read only. >> >> The part of my draft patch about this: >> + fn_decl = gimple_call_fndecl (stmt); >> + /* ??? Do the same for ITM memcpy/memmove */ >> + if (is_tm_memset (fn_decl)) >> + transaction_subcode_ior (region, GTMA_HAVE_STORE); > > Actually, if you look a few lines down, you will notice that we always > set GTMA_HAVE_{STORE,LOAD} for non TM pure calls: > > /* Note that something may happen. */ > *state |= GTMA_HAVE_LOAD | GTMA_HAVE_STORE; > > So the transaction will never be marked as read-only incorrectly. You are talking about the function examine_call_tm(), I was talking about is in expand_call_tm(). Anyway I sent a testcase with the patch which fails without the modification to expand_call_tm(). Patrick. Here (again) the testcase. /* { dg-do compile } */ /* { dg-options "-fgnu-tm -fdump-tree-tmedge -fdump-tree-tmlower" } */ char array[4] = "aaaa"; void *memset(void *s, int c, unsigned long int n); int main() { __transaction [[atomic]] { memset(array, 'b', sizeof(4)); } } /* { dg-final { scan-tree-dump-times "readOnly" 0 "tmedge" } } */ /* { dg-final { scan-tree-dump-times "GTMA_HAVE_STORE" 1 "tmlower" } } */ /* { dg-final { cleanup-tree-dump "tmedge" } } */ /* { dg-final { cleanup-tree-dump "tmlower" } } */
On 01/31/2011 10:20 AM, Aldy Hernandez wrote: > PR/47492 > * tree-ssa-alias.c (ref_maybe_used_by_call_p_1): Handle > BUILT_IN_TM_MEMSET. > * trans-mem.c (find_tm_replacement_function): Same. > * calls.c (special_function_p): Same. > * tree-ssa-structalias.c: Same. > * gtm-builtins.def: Add BUILT_IN_TM_MEMSET. Ok. Patrick's test case at the end of this thread can be handled with a separate one liner. r~
Index: testsuite/gcc.dg/tm/memset.c =================================================================== --- testsuite/gcc.dg/tm/memset.c (revision 0) +++ testsuite/gcc.dg/tm/memset.c (revision 0) @@ -0,0 +1,24 @@ +/* { dg-do compile } */ +/* { dg-options "-fgnu-tm" } */ + +extern void *memset (void *, int, __SIZE_TYPE__); + +char array[4] = "aaaa"; + +__attribute__((transaction_safe)) +void *my_memset() +{ + return memset(array,'b',4); +} + + +int main() +{ + + __transaction [[atomic]] { + my_memset(); + } + return 0; +} + +/* { dg-final { scan-assembler "_ITM_memsetW" } } */ Index: tree-ssa-alias.c =================================================================== --- tree-ssa-alias.c (revision 169292) +++ tree-ssa-alias.c (working copy) @@ -1057,6 +1057,7 @@ ref_maybe_used_by_call_p_1 (gimple call, case BUILT_IN_MALLOC: case BUILT_IN_CALLOC: case BUILT_IN_MEMSET: + case BUILT_IN_TM_MEMSET: case BUILT_IN_FREXP: case BUILT_IN_FREXPF: case BUILT_IN_FREXPL: @@ -1234,6 +1235,7 @@ call_may_clobber_ref_p_1 (gimple call, a case BUILT_IN_STRCAT: case BUILT_IN_STRNCAT: case BUILT_IN_MEMSET: + case BUILT_IN_TM_MEMSET: CASE_BUILT_IN_TM_STORE (1): CASE_BUILT_IN_TM_STORE (2): CASE_BUILT_IN_TM_STORE (4): Index: trans-mem.c =================================================================== --- trans-mem.c (revision 169442) +++ trans-mem.c (working copy) @@ -485,6 +485,8 @@ find_tm_replacement_function (tree fndec return built_in_decls[BUILT_IN_TM_MEMCPY]; case BUILT_IN_MEMMOVE: return built_in_decls[BUILT_IN_TM_MEMMOVE]; + case BUILT_IN_MEMSET: + return built_in_decls[BUILT_IN_TM_MEMSET]; default: return NULL; } Index: calls.c =================================================================== --- calls.c (revision 169292) +++ calls.c (working copy) @@ -480,6 +480,7 @@ special_function_p (const_tree fndecl, i case BUILT_IN_TM_GETTMCLONE_IRR: case BUILT_IN_TM_MEMCPY: case BUILT_IN_TM_MEMMOVE: + case BUILT_IN_TM_MEMSET: CASE_BUILT_IN_TM_STORE (1): CASE_BUILT_IN_TM_STORE (2): CASE_BUILT_IN_TM_STORE (4): Index: gtm-builtins.def =================================================================== --- gtm-builtins.def (revision 169292) +++ gtm-builtins.def (working copy) @@ -14,6 +14,8 @@ DEF_TM_BUILTIN (BUILT_IN_TM_MEMCPY, "_IT BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_TM_TMPURE_NOTHROW_LIST) DEF_TM_BUILTIN (BUILT_IN_TM_MEMMOVE, "_ITM_memmoveRtWt", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_TM_TMPURE_NOTHROW_LIST) +DEF_TM_BUILTIN (BUILT_IN_TM_MEMSET, "_ITM_memsetW", + BT_FN_PTR_PTR_INT_SIZE, ATTR_TM_TMPURE_NOTHROW_LIST) DEF_TM_BUILTIN (BUILT_IN_TM_GETTMCLONE_IRR, "_ITM_getTMCloneOrIrrevocable", BT_FN_PTR_PTR, ATTR_TM_CONST_NOTHROW_LIST) Index: tree-ssa-structalias.c =================================================================== --- tree-ssa-structalias.c (revision 169292) +++ tree-ssa-structalias.c (working copy) @@ -3720,6 +3720,7 @@ find_func_aliases (gimple t) return; } case BUILT_IN_MEMSET: + case BUILT_IN_TM_MEMSET: { tree res = gimple_call_lhs (t); tree dest = gimple_call_arg (t, 0);