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

[Richard Guenther <richard.guenther@gmail.com>]

On Sun, Nov 6, 2011 at 7:53 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> [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?

Do not push new_version->lowered setting into the core routine but keep
it at the callers.

>>> +  /* 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.

It just catched my eye...  moving it to expand_call_stmt would be nice
indeed, but I was suggesting to add that note where we produce the
CALL rtx, not sure if that's reasonably straight-forward (I suppose there
was a reason to go with the hack above ...).

Richard.