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

[Aldy Hernandez <aldyh at redhat dot com>]

[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: curr
Description: Text document