Patchwork [trans-mem] handle memset (PR/47492)

login
register
mail settings
Submitter Aldy Hernandez
Date Jan. 31, 2011, 6:20 p.m.
Message ID <4D46FD58.5050207@redhat.com>
Download mbox | patch
Permalink /patch/81201/
State New
Headers show

Comments

Aldy Hernandez - Jan. 31, 2011, 6:20 p.m.
Transform memset into its transactional equivalent.

Thanks to Patrick for doing all the dirty work on this patch.

OK for branch?
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.
Patrick Marlier - Jan. 31, 2011, 10:17 p.m.
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?
>
Torvald Riegel - Feb. 1, 2011, 8:33 a.m.
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
Aldy Hernandez - Feb. 2, 2011, 3:41 p.m.
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?
Patrick Marlier - Feb. 2, 2011, 4:35 p.m.
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" } } */
Richard Henderson - Feb. 2, 2011, 4:45 p.m.
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~

Patch

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);