Re: [patch] 19/n: trans-mem: middle end/misc patches (LAST PATCH)

[Aldy Hernandez <aldyh@redhat.com>]

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

[rth, more comments for you below]

On 11/04/11 04:14, Richard Guenther wrote:

>>     new_version = cgraph_create_node (new_decl);
>>
>> -   new_version->analyzed = true;
>> +   new_version->analyzed = old_version->analyzed;
>
> Hm?  analyzed means "with body", sure you have a body if you clone.
>
>>     new_version->local = old_version->local;
>>     new_version->local.externally_visible = false;
>>     new_version->local.local = true;
>> @@ -2294,6 +2294,7 @@ cgraph_copy_node_for_versioning (struct
>>     new_version->rtl = old_version->rtl;
>>     new_version->reachable = true;
>>     new_version->count = old_version->count;
>> +   new_version->lowered = true;
>
> OTOH this isn't necessary true.  cgraph exists before lowering.

I don't understand what you want me to do on either of these two 
comments.  Could you elaborate?

>> +  /* TM pure functions should not get inlined if the outer function is
>> +     a TM safe function.  */
>> +  else if (flag_tm
>
> Please move flag checks into the respective prediates.  Any reason
> why the is_tm_pure () predicate wouldn't already do the correct thing
> with !flag_tm?

Done.

>> +  /* Map gimple stmt to tree label (or list of labels) for transaction
>> +     restart and abort.  */
>> +  htab_t GTY ((param_is (struct tm_restart_node))) tm_restart;
>> +
>
> As this maps 'gimple' to tree shouldn't this go to fn->gimple_df instead?
> That way you avoid growing generic struct function.  Or in to eh_status,
> if that looks like a better fit.

Done.

>> +  /* Mark all calls that can have a transaction restart.  */
>
> Why isn't this done when we expand the call?  This walking of the
> RTL sequence looks like a hack (an easy one, albeit).
>
>> +  if (cfun->tm_restart&&  is_gimple_call (stmt))
>> +    {
>> +      struct tm_restart_node dummy;
>> +      void **slot;
>> +
>> +      dummy.stmt = stmt;
>> +      slot = htab_find_slot (cfun->tm_restart,&dummy, NO_INSERT);
>> +      if (slot)
>> +       {
>> +         struct tm_restart_node *n = (struct tm_restart_node *) *slot;
>> +         tree list = n->label_or_list;
>> +         rtx insn;
>> +
>> +         for (insn = next_real_insn (last); !CALL_P (insn);
>> +              insn = next_real_insn (insn))
>> +           continue;
>> +
>> +         if (TREE_CODE (list) == LABEL_DECL)
>> +           add_reg_note (insn, REG_TM, label_rtx (list));
>> +         else
>> +           for (; list ; list = TREE_CHAIN (list))
>> +             add_reg_note (insn, REG_TM, label_rtx (TREE_VALUE (list)));
>> +       }
>> +    }

I can certainly move this to expand_call_stmt() if you prefer.  Do you 
have an objection to the RTL walk?  This isn't my code, but I'm open to 
suggestions on an alternative to implement.

>> +  /* After expanding, the tm_restart map is no longer needed.  */
>> +  cfun->tm_restart = NULL;
>
> You should still free it, to not confuse the statistics code I think.

Done.

>> +finish_tm_clone_pairs (void)
>> +{
>> +  bool switched = false;
>> +
>> +  if (tm_clone_pairs == NULL)
>> +    return;
>> +
>> +  htab_traverse_noresize (tm_clone_pairs, finish_tm_clone_pairs_1,
>> +                         (void *)&switched);
>
> This makes the generated table dependent on memory layout.  You
> need to walk the pairs in some deterministic order.  In fact why not
> walk all cgraph_nodes looking for the pairs - they should be still
> in the list of clones for a node and you've marked it with DECL_TM_CLONE.
> You can then sort them by cgraph node uid.
>
> Did you check bootstrapping GCC with TM enabled and address-space
> randomization turned on?

Actually, the table organization is irrelevant, because upon registering 
of the table in the runtime, we qsort the entire thing.  We then do a 
binary search to find items.  See _ITM_registerTMCloneTable() and 
find_clone() in the libitm runtime.

>> +/* In gtm-low.c  */
>> +extern bool is_transactional_stmt (const_gimple);
>> +
>
> gimple.h please.  looks like a gimple predicate as well, so the implementation
> should be in gimple.c?

Woo hoo!  Unused function.  I've removed it altogether.

>>         case GIMPLE_GOTO:
>> -          if (!computed_goto_p (stmt))
>> +         if (!computed_goto_p (stmt))
>>             {
>> -             tree new_dest = main_block_label (gimple_goto_dest (stmt));
>> -             gimple_goto_set_dest (stmt, new_dest);
>> +             label = gimple_goto_dest (stmt);
>> +             new_label = main_block_label (label);
>> +             if (new_label != label)
>> +               gimple_goto_set_dest (stmt, new_label);
>
> What's the reason for this changes?  Optimization?

Yes.  Rth can elaborate if you deem necessary.

>> +/* Verify the contents of a GIMPLE_TRANSACTION.  Returns true if there
>> +   is a problem, otherwise false.  */
>> +
>> +static bool
>> +verify_gimple_transaction (gimple stmt)
>> +{
>> +  tree lab = gimple_transaction_label (stmt);
>> +  if (lab != NULL&&  TREE_CODE (lab) != LABEL_DECL)
>> +    return true;
>
> ISTR this has substatements, so you should handle this in
> verify_gimple_in_seq_2 and make sure to verify those substatements.

I have added verification for the substatements in 
verify_gimple_transaction()...

>> @@ -4155,6 +4210,9 @@ verify_gimple_stmt (gimple stmt)
>>      case GIMPLE_ASM:
>>        return false;
>>
>> +    case GIMPLE_TRANSACTION:
>> +      return verify_gimple_transaction (stmt);
>> +
>
> Not here.

...but we still need to check the transaction in verify_gimple_stmt() 
(as well as in verify_gimple_in_seq_2), because verify_gimple_in_cfg() 
will verify a gimple_transaction by calling verify_gimple_stmt, so we 
must handle GIMPLE_TRANSACTION in verify_gimple_stmt also.

>> +       case GIMPLE_TRANSACTION:
>> +         err |= verify_gimple_in_seq_2 (gimple_transaction_body (stmt));
>> +         break;

I am calling verify_gimple_transaction() now.  See patch.

>> +    case GIMPLE_TRANSACTION:
>> +      /* The ABORT edge has a stored label associated with it, otherwise
>> +        the edges are simply redirectable.  */
>> +      /* ??? We don't really need this label after the cfg is created.  */
>> +      if (e->flags == 0)
>> +       gimple_transaction_set_label (stmt, gimple_block_label (dest));
>
> So why set it (and thus keep it live)?

This seems like leftovers from a previous incantation.  However, I'm not 
100% sure, so I have disabled the code, but left it in a comment.  A 
full bootstrap/regtest revealed no regressions.

rth, do you have any objections to remove this?

Tested on x86-64 Linux.

OK for branch?

p.s. This changelog entry is for ChangeLog.tm, but I will adapt a merged 
ChangeLog entry for ChangeLog.tm-merge as well.  And will do so from now on.

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 9333 bytes --]

	* tree-cfg.c (verify_gimple_transaction): Verify body.  Move down.
	(verify_gimple_in_seq_2): Verify the label in a
	GIMPLE_TRANSACTION.
	* function.h (struct function): Move tm_restart field to struct
	gimple_df in tree-flow.h
	Move tm_restart_node to tree-flow.h
	* tree-flow.h (struct gimple_df): New location for tm_restart
	field.
	New location for tm_restart_node.
	(is_transactional_stmt): Remove.
	* trans-mem.c (is_transactional_stmt): Remove.
	(make_tm_edge): Field tm_restart is now in gimple_df.
	* cfgexpand.c (gimple_expand_cfg): Field tm_restart is now in
	cfun->gimple_df.
	Free tm_restart.
	* cfgexpand.c (expand_gimple_stmt): Field tm_restart is now in
	gimple_df.
	* ipa-inline.c (can_inline_edge_p): Do not check flag_tm.
	* trans-mem.c (is_tm_pure): Check flag_tm.
	(is_tm_safe): Same.

Index: ipa-inline.c
===================================================================
--- ipa-inline.c	(revision 181028)
+++ ipa-inline.c	(working copy)
@@ -286,8 +286,7 @@ can_inline_edge_p (struct cgraph_edge *e
     }
   /* TM pure functions should not get inlined if the outer function is
      a TM safe function.  */
-  else if (flag_tm
-	   && is_tm_pure (callee->decl)
+  else if (is_tm_pure (callee->decl)
 	   && is_tm_safe (e->caller->decl))
     {
       e->inline_failed = CIF_UNSPECIFIED;
Index: function.h
===================================================================
--- function.h	(revision 181028)
+++ function.h	(working copy)
@@ -467,14 +467,6 @@ extern GTY(()) struct rtl_data x_rtl;
    want to do differently.  */
 #define crtl (&x_rtl)
 
-/* This structure is used to map a gimple statement to a label,
-   or list of labels to represent transaction restart.  */
-
-struct GTY(()) tm_restart_node {
-  gimple stmt;
-  tree label_or_list;
-};
-
 struct GTY(()) stack_usage
 {
   /* # of bytes of static stack space allocated by the function.  */
@@ -526,10 +518,6 @@ struct GTY(()) function {
   /* Value histograms attached to particular statements.  */
   htab_t GTY((skip)) value_histograms;
 
-  /* Map gimple stmt to tree label (or list of labels) for transaction
-     restart and abort.  */
-  htab_t GTY ((param_is (struct tm_restart_node))) tm_restart;
-
   /* For function.c.  */
 
   /* Points to the FUNCTION_DECL of this function.  */
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 181028)
+++ trans-mem.c	(working copy)
@@ -172,9 +172,13 @@ get_attrs_for (const_tree x)
 bool
 is_tm_pure (const_tree x)
 {
-  tree attrs = get_attrs_for (x);
-  if (attrs)
-    return lookup_attribute ("transaction_pure", attrs) != NULL;
+  if (flag_tm)
+    {
+      tree attrs = get_attrs_for (x);
+      if (attrs)
+	return lookup_attribute ("transaction_pure", attrs) != NULL;
+      return false;
+    }
   return false;
 }
 
@@ -205,16 +209,17 @@ is_tm_irrevocable (tree x)
 bool
 is_tm_safe (const_tree x)
 {
-  tree attrs = get_attrs_for (x);
-
-  if (attrs)
+  if (flag_tm)
     {
-      if (lookup_attribute ("transaction_safe", attrs))
-	return true;
-      if (lookup_attribute ("transaction_may_cancel_outer", attrs))
-	return true;
+      tree attrs = get_attrs_for (x);
+      if (attrs)
+	{
+	  if (lookup_attribute ("transaction_safe", attrs))
+	    return true;
+	  if (lookup_attribute ("transaction_may_cancel_outer", attrs))
+	    return true;
+	}
     }
-
   return false;
 }
 
@@ -270,22 +275,6 @@ is_tm_may_cancel_outer (tree x)
   return false;
 }
 
-/* Return true if STMT may alter control flow via a transactional edge.  */
-
-bool
-is_transactional_stmt (const_gimple stmt)
-{
-  switch (gimple_code (stmt))
-    {
-    case GIMPLE_CALL:
-      return (gimple_call_flags (stmt) & ECF_TM_OPS) != 0;
-    case GIMPLE_TRANSACTION:
-      return true;
-    default:
-      return false;
-    }
-}
-
 /* Return true for built in functions that "end" a transaction.   */
 
 bool
@@ -2470,13 +2459,13 @@ make_tm_edge (gimple stmt, basic_block b
   void **slot;
   struct tm_restart_node *n, dummy;
 
-  if (cfun->tm_restart == NULL)
-    cfun->tm_restart = htab_create_ggc (31, struct_ptr_hash,
-					struct_ptr_eq, ggc_free);
+  if (cfun->gimple_df->tm_restart == NULL)
+    cfun->gimple_df->tm_restart = htab_create_ggc (31, struct_ptr_hash,
+						   struct_ptr_eq, ggc_free);
 
   dummy.stmt = stmt;
   dummy.label_or_list = gimple_block_label (region->entry_block); 
-  slot = htab_find_slot (cfun->tm_restart, &dummy, INSERT);
+  slot = htab_find_slot (cfun->gimple_df->tm_restart, &dummy, INSERT);
   n = (struct tm_restart_node *) *slot;
   if (n == NULL)
     {
Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 181028)
+++ cfgexpand.c	(working copy)
@@ -2097,13 +2097,13 @@ expand_gimple_stmt (gimple stmt)
     }
 
   /* Mark all calls that can have a transaction restart.  */
-  if (cfun->tm_restart && is_gimple_call (stmt))
+  if (cfun->gimple_df->tm_restart && is_gimple_call (stmt))
     {
       struct tm_restart_node dummy;
       void **slot;
 
       dummy.stmt = stmt;
-      slot = htab_find_slot (cfun->tm_restart, &dummy, NO_INSERT);
+      slot = htab_find_slot (cfun->gimple_df->tm_restart, &dummy, NO_INSERT);
       if (slot)
 	{
 	  struct tm_restart_node *n = (struct tm_restart_node *) *slot;
@@ -4483,7 +4483,11 @@ gimple_expand_cfg (void)
   naked_return_label = NULL;
 
   /* After expanding, the tm_restart map is no longer needed.  */
-  cfun->tm_restart = NULL;
+  if (cfun->gimple_df->tm_restart)
+    {
+      htab_delete (cfun->gimple_df->tm_restart);
+      cfun->gimple_df->tm_restart = NULL;
+    }
 
   /* Tag the blocks with a depth number so that change_scope can find
      the common parent easily.  */
Index: tree-flow.h
===================================================================
--- tree-flow.h	(revision 181028)
+++ tree-flow.h	(working copy)
@@ -33,6 +33,14 @@ along with GCC; see the file COPYING3.  
 #include "tree-ssa-alias.h"
 
 
+/* This structure is used to map a gimple statement to a label,
+   or list of labels to represent transaction restart.  */
+
+struct GTY(()) tm_restart_node {
+  gimple stmt;
+  tree label_or_list;
+};
+
 /* Gimple dataflow datastructure. All publicly available fields shall have
    gimple_ accessor defined in tree-flow-inline.h, all publicly modifiable
    fields should have gimple_set accessor.  */
@@ -80,6 +88,10 @@ struct GTY(()) gimple_df {
   unsigned int ipa_pta : 1;
 
   struct ssa_operands ssa_operands;
+
+  /* Map gimple stmt to tree label (or list of labels) for transaction
+     restart and abort.  */
+  htab_t GTY ((param_is (struct tm_restart_node))) tm_restart;
 };
 
 /* Accessors for internal use only.  Generic code should use abstraction
@@ -778,9 +790,6 @@ extern bool maybe_duplicate_eh_stmt (gim
 extern bool verify_eh_edges (gimple);
 extern bool verify_eh_dispatch_edge (gimple);
 
-/* In gtm-low.c  */
-extern bool is_transactional_stmt (const_gimple);
-
 /* In tree-ssa-pre.c  */
 struct pre_expr_d;
 void add_to_value (unsigned int, struct pre_expr_d *);
Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 181028)
+++ tree-cfg.c	(working copy)
@@ -117,6 +117,7 @@ static int gimple_verify_flow_info (void
 static void gimple_make_forwarder_block (edge);
 static void gimple_cfg2vcg (FILE *);
 static gimple first_non_label_stmt (basic_block);
+static bool verify_gimple_transaction (gimple);
 
 /* Flowgraph optimization and cleanup.  */
 static void gimple_merge_blocks (basic_block, basic_block);
@@ -4098,18 +4099,6 @@ verify_gimple_switch (gimple stmt)
   return false;
 }
 
-/* Verify the contents of a GIMPLE_TRANSACTION.  Returns true if there
-   is a problem, otherwise false.  */
-
-static bool
-verify_gimple_transaction (gimple stmt)
-{
-  tree lab = gimple_transaction_label (stmt);
-  if (lab != NULL && TREE_CODE (lab) != LABEL_DECL)
-    return true;
-  return false;
-}
-
 /* Verify a gimple debug statement STMT.
    Returns true if anything is wrong.  */
 
@@ -4339,7 +4328,7 @@ verify_gimple_in_seq_2 (gimple_seq stmts
 	  break;
 
 	case GIMPLE_TRANSACTION:
-	  err |= verify_gimple_in_seq_2 (gimple_transaction_body (stmt));
+	  err |= verify_gimple_transaction (stmt);
 	  break;
 
 	default:
@@ -4355,6 +4344,18 @@ verify_gimple_in_seq_2 (gimple_seq stmts
   return err;
 }
 
+/* Verify the contents of a GIMPLE_TRANSACTION.  Returns true if there
+   is a problem, otherwise false.  */
+
+static bool
+verify_gimple_transaction (gimple stmt)
+{
+  tree lab = gimple_transaction_label (stmt);
+  if (lab != NULL && TREE_CODE (lab) != LABEL_DECL)
+    return true;
+  return verify_gimple_in_seq_2 (gimple_transaction_body (stmt));
+}
+
 
 /* Verify the GIMPLE statements inside the statement list STMTS.  */
 
@@ -5122,9 +5123,10 @@ gimple_redirect_edge_and_branch (edge e,
     case GIMPLE_TRANSACTION:
       /* The ABORT edge has a stored label associated with it, otherwise
 	 the edges are simply redirectable.  */
-      /* ??? We don't really need this label after the cfg is created.  */
-      if (e->flags == 0)
+      /* ??? We don't really need this label after the cfg is created.
+      if (&e->flags == 0)
 	gimple_transaction_set_label (stmt, gimple_block_label (dest));
+      */
       break;
 
     default: