On Sun, Nov 6, 2011 at 7:53 PM, Aldy Hernandez <al...@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.
