Re: [patch] 19/n: trans-mem: compiler tree/gimple stuff

[Richard Henderson <rth@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1994 bytes --]

On 11/05/2011 03:09 PM, Richard Guenther wrote:
> On Sat, Nov 5, 2011 at 10:05 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
>> [rth, see below]
>>
>>>>   local_define_builtin ("__builtin_eh_pointer", ftype,
>>>> BUILT_IN_EH_POINTER,
>>>>                        "__builtin_eh_pointer", ECF_PURE | ECF_NOTHROW |
>>>> ECF_LEAF);
>>>> +  if (flag_tm)
>>>> +    apply_tm_attr (builtin_decl_explicit (BUILT_IN_EH_POINTER),
>>>> +                  get_identifier ("transaction_pure"));
>>>
>>> I think this should use a new ECF_TM_PURE flag, unconditionally set
>>> with handling in the functions that handle/return ECF flags so that
>>> transitioning this to a tree node flag instead of an attribute is easier.
>>
>> I could add a ECF_TM_PURE flag and attach it to the BUILT_IN_EH_POINTER in
>> the local_define_builtin above, but we still need the attribute for function
>> decl's as in:
>>
>> __attribute__((transaction_pure)) void foo();
>>
>> Attributes seem like a clean way to approach this.
> 
> The middle-end interfacing is supposed to be via ECF_ flags, the user interface
> via attributes.  What's the semantic of transaction-pure vs. ...
> 
>> I don't see what the flag buys us.  Or am I misunderstanding something?
>>
>>>> +/* Nonzero if this call performs a transactional memory operation.  */
>>>> +#define ECF_TM_OPS               (1<<  11)
>>>
>>> What's this flag useful for?  Isn't it the case that you want to
>>> conservatively
>>> know whether a call might perform a tm operation?  Thus, the flag
>>> should be inverted?  Is this the same as "TM pure"?
> 
> ... this?
> 
>> Richard?
> 
>> Richi, I have fixed or addressed all the issues in this thread, with the
>> exception of your EFC_TM_PURE and ECF_TM_OPS questions, which I am deferring
>> to rth and then fixing if required.
> 
> Yeah, seems to be still an open question.

I hope this cleanup both addresses the above questions and tidies things
up as indicated.  Please ask if you've got more questions.

Ok, Richi?


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 7520 bytes --]

diff --git a/gcc/calls.c b/gcc/calls.c
index 515ab97..382de7f 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -707,13 +707,28 @@ flags_from_decl_or_type (const_tree exp)
       if (TREE_NOTHROW (exp))
 	flags |= ECF_NOTHROW;
 
-      if (is_tm_builtin (exp))
-	flags |= ECF_TM_OPS;
+      if (flag_tm)
+	{
+	  if (is_tm_builtin (exp))
+	    flags |= ECF_TM_BUILTIN;
+	  else if ((flags & ECF_CONST) != 0
+		   || lookup_attribute ("transaction_pure",
+					TYPE_ATTRIBUTES (TREE_TYPE (exp))))
+	    flags |= ECF_TM_PURE;
+	}
 
       flags = special_function_p (exp, flags);
     }
-  else if (TYPE_P (exp) && TYPE_READONLY (exp))
-    flags |= ECF_CONST;
+  else if (TYPE_P (exp))
+    {
+      if (TYPE_READONLY (exp))
+	flags |= ECF_CONST;
+
+      if (flag_tm
+	  && ((flags & ECF_CONST) != 0
+	      || lookup_attribute ("transaction_pure", TYPE_ATTRIBUTES (exp))))
+	flags |= ECF_TM_PURE;
+    }
 
   if (TREE_THIS_VOLATILE (exp))
     {
diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c
index ba25fd8..be399a0 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -172,14 +172,8 @@ get_attrs_for (const_tree x)
 bool
 is_tm_pure (const_tree x)
 {
-  if (flag_tm)
-    {
-      tree attrs = get_attrs_for (x);
-      if (attrs)
-	return lookup_attribute ("transaction_pure", attrs) != NULL;
-      return false;
-    }
-  return false;
+  unsigned flags = flags_from_decl_or_type (x);
+  return (flags & ECF_TM_PURE) != 0;
 }
 
 /* Return true if X has been marked TM_IRREVOCABLE.  */
@@ -229,10 +223,6 @@ static bool
 is_tm_pure_call (gimple call)
 {
   tree fn = gimple_call_fn (call);
-  unsigned flags;
-
-  if (is_tm_pure (TREE_TYPE (fn)))
-    return true;
 
   if (TREE_CODE (fn) == ADDR_EXPR)
     {
@@ -241,9 +231,8 @@ is_tm_pure_call (gimple call)
     }
   else
     fn = TREE_TYPE (fn);
-  flags = flags_from_decl_or_type (fn);
 
-  return (flags & ECF_CONST) != 0;
+  return is_tm_pure (fn);
 }
 
 /* Return true if X has been marked TM_CALLABLE.  */
@@ -2484,7 +2473,7 @@ make_tm_edge (gimple stmt, basic_block bb, struct tm_region *region)
 }
 
 
-/* Split block BB as necessary for every TM_OPS function we added, and
+/* Split block BB as necessary for every builtin function we added, and
    wire up the abnormal back edges implied by the transaction restart.  */
 
 static void
@@ -2496,15 +2485,16 @@ expand_block_edges (struct tm_region *region, basic_block bb)
     {
       gimple stmt = gsi_stmt (gsi);
 
-      /* ??? TM_COMMIT (and any other ECF_TM_OPS function) in a nested
+      /* ??? TM_COMMIT (and any other tm builtin function) in a nested
 	 transaction has an abnormal edge back to the outer-most transaction
 	 (there are no nested retries), while a TM_ABORT also has an abnormal
 	 backedge to the inner-most transaction.  We haven't actually saved
 	 the inner-most transaction here.  We should be able to get to it
 	 via the region_nr saved on STMT, and read the transaction_stmt from
 	 that, and find the first region block from there.  */
+      /* ??? Shouldn't we split for any non-pure, non-irrevocable function?  */
       if (gimple_code (stmt) == GIMPLE_CALL
-	  && (gimple_call_flags (stmt) & ECF_TM_OPS) != 0)
+	  && (gimple_call_flags (stmt) & ECF_TM_BUILTIN) != 0)
 	{
 	  if (gsi_one_before_end_p (gsi))
 	    make_tm_edge (stmt, bb, region);
@@ -3934,11 +3924,18 @@ ipa_tm_mayenterirr_function (struct cgraph_node *node)
 {
   struct tm_ipa_cg_data *d = get_cg_data (node);
   tree decl = node->decl;
+  unsigned flags = flags_from_decl_or_type (decl);
+
+  /* Handle some TM builtins.  Ordinarily these aren't actually generated
+     at this point, but handling these functions when written in by the
+     user makes it easier to build unit tests.  */
+  if (flags & ECF_TM_BUILTIN)
+    return false;
 
   /* Filter out all functions that are marked.  */
-  if (is_tm_safe_or_pure (decl))
+  if (flags & ECF_TM_PURE)
     return false;
-  if ((flags_from_decl_or_type (decl) & ECF_CONST) != 0)
+  if (is_tm_safe (decl))
     return false;
   if (is_tm_irrevocable (decl))
     return true;
@@ -3947,11 +3944,6 @@ ipa_tm_mayenterirr_function (struct cgraph_node *node)
   if (find_tm_replacement_function (decl))
     return true;
 
-  /* Handle some TM builtins.  */
-  if (DECL_BUILT_IN_CLASS (decl) == BUILT_IN_NORMAL
-      && (flags_from_decl_or_type (decl) & ECF_TM_OPS) != 0)
-    return false;
-
   /* If we aren't seeing the final version of the function we don't
      know what it will contain at runtime.  */
   if (cgraph_function_body_availability (node) < AVAIL_AVAILABLE)
@@ -4394,8 +4386,10 @@ ipa_tm_transform_calls_redirect (struct cgraph_node *node,
       return;
     }
 
-  /* If the call is to the TM runtime, do nothing further.  */
-  if (flags_from_decl_or_type (fndecl) & ECF_TM_OPS)
+  /* Handle some TM builtins.  Ordinarily these aren't actually generated
+     at this point, but handling these functions when written in by the
+     user makes it easier to build unit tests.  */
+  if (flags_from_decl_or_type (fndecl) & ECF_TM_BUILTIN)
     return;
 
   /* Fixup recursive calls inside clones.  */
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 61e4476..7cb4a3d 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -2298,9 +2298,9 @@ is_ctrl_altering_stmt (gimple t)
 	  return true;
 
 	/* TM ending statements have backedges out of the transaction.
-	   Return true so we split the basic block containing
-	   them.  */
-	if ((flags & ECF_TM_OPS)
+	   Return true so we split the basic block containing them.
+	   Note that the TM_BUILTIN test is merely an optimization.  */
+	if ((flags & ECF_TM_BUILTIN)
 	    && is_tm_ending_fndecl (gimple_call_fndecl (t)))
 	  return true;
 
diff --git a/gcc/tree.c b/gcc/tree.c
index 30c6bb8..ba6c2e1 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -9428,6 +9428,8 @@ local_define_builtin (const char *name, tree type, enum built_in_function code,
   if (ecf_flags & ECF_LEAF)
     DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("leaf"),
 					NULL, DECL_ATTRIBUTES (decl));
+  if ((ecf_flags & ECF_TM_PURE) && flag_tm)
+    apply_tm_attr (decl, get_identifier ("transaction_pure"));
 
   set_builtin_decl (code, decl, true);
 }
@@ -9593,10 +9595,8 @@ build_common_builtin_nodes (void)
   ftype = build_function_type_list (ptr_type_node,
 				    integer_type_node, NULL_TREE);
   local_define_builtin ("__builtin_eh_pointer", ftype, BUILT_IN_EH_POINTER,
-			"__builtin_eh_pointer", ECF_PURE | ECF_NOTHROW | ECF_LEAF);
-  if (flag_tm)
-    apply_tm_attr (builtin_decl_explicit (BUILT_IN_EH_POINTER),
-		   get_identifier ("transaction_pure"));
+			"__builtin_eh_pointer",
+			ECF_PURE | ECF_NOTHROW | ECF_LEAF | ECF_TM_PURE);
 
   tmp = lang_hooks.types.type_for_mode (targetm.eh_return_filter_mode (), 0);
   ftype = build_function_type_list (tmp, integer_type_node, NULL_TREE);
diff --git a/gcc/tree.h b/gcc/tree.h
index 23f3d69..ab20272 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -5601,8 +5601,10 @@ extern tree build_duplicate_type (tree);
 #define ECF_NOVOPS		  (1 << 9)
 /* The function does not lead to calls within current function unit.  */
 #define ECF_LEAF		  (1 << 10)
-/* Nonzero if this call performs a transactional memory operation.  */
-#define ECF_TM_OPS		  (1 << 11)
+/* Nonzero if this call does not affect transactions.  */
+#define ECF_TM_PURE		  (1 << 11)
+/* Nonzero if this call is into the transaction runtime library.  */
+#define ECF_TM_BUILTIN		  (1 << 12)
 
 extern int flags_from_decl_or_type (const_tree);
 extern int call_expr_flags (const_tree);