[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.