[patch] 19/n: trans-mem: middle end/misc patches (LAST PATCH)
Aldy Hernandez
aldyh@redhat.com
Sun Nov 6 19:07:00 GMT 2011
[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.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: curr
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20111106/a0b53e7e/attachment.ksh>
More information about the Gcc-patches
mailing list