From patchwork Sun Nov 6 18:53:11 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 123957 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id B5058B6F75 for ; Mon, 7 Nov 2011 05:53:40 +1100 (EST) Received: (qmail 10042 invoked by alias); 6 Nov 2011 18:53:38 -0000 Received: (qmail 10032 invoked by uid 22791); 6 Nov 2011 18:53:36 -0000 X-SWARE-Spam-Status: No, hits=-7.1 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, SPF_HELO_PASS, TW_CF, TW_TM X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 06 Nov 2011 18:53:15 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pA6IrE37007192 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 6 Nov 2011 13:53:14 -0500 Received: from austin.quesejoda.com (vpn-11-235.rdu.redhat.com [10.11.11.235]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id pA6IrC64024851; Sun, 6 Nov 2011 13:53:13 -0500 Message-ID: <4EB6D797.4070309@redhat.com> Date: Sun, 06 Nov 2011 10:53:11 -0800 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.1.10-1.fc14 Lightning/1.0b3pre Thunderbird/3.1.10 MIME-Version: 1.0 To: Richard Guenther CC: gcc-patches , Richard Henderson Subject: Re: [patch] 19/n: trans-mem: middle end/misc patches (LAST PATCH) References: <4EB2EC3F.6000908@redhat.com> In-Reply-To: Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org [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. * tree-cfg.c (verify_gimple_transaction): Verify body. Move down. (verify_gimple_in_seq_2): Verify the label in a GIMPLE_TRANSACTION. * function.h (struct function): Move tm_restart field to struct gimple_df in tree-flow.h Move tm_restart_node to tree-flow.h * tree-flow.h (struct gimple_df): New location for tm_restart field. New location for tm_restart_node. (is_transactional_stmt): Remove. * trans-mem.c (is_transactional_stmt): Remove. (make_tm_edge): Field tm_restart is now in gimple_df. * cfgexpand.c (gimple_expand_cfg): Field tm_restart is now in cfun->gimple_df. Free tm_restart. * cfgexpand.c (expand_gimple_stmt): Field tm_restart is now in gimple_df. * ipa-inline.c (can_inline_edge_p): Do not check flag_tm. * trans-mem.c (is_tm_pure): Check flag_tm. (is_tm_safe): Same. Index: ipa-inline.c =================================================================== --- ipa-inline.c (revision 181028) +++ ipa-inline.c (working copy) @@ -286,8 +286,7 @@ can_inline_edge_p (struct cgraph_edge *e } /* TM pure functions should not get inlined if the outer function is a TM safe function. */ - else if (flag_tm - && is_tm_pure (callee->decl) + else if (is_tm_pure (callee->decl) && is_tm_safe (e->caller->decl)) { e->inline_failed = CIF_UNSPECIFIED; Index: function.h =================================================================== --- function.h (revision 181028) +++ function.h (working copy) @@ -467,14 +467,6 @@ extern GTY(()) struct rtl_data x_rtl; want to do differently. */ #define crtl (&x_rtl) -/* This structure is used to map a gimple statement to a label, - or list of labels to represent transaction restart. */ - -struct GTY(()) tm_restart_node { - gimple stmt; - tree label_or_list; -}; - struct GTY(()) stack_usage { /* # of bytes of static stack space allocated by the function. */ @@ -526,10 +518,6 @@ struct GTY(()) function { /* Value histograms attached to particular statements. */ htab_t GTY((skip)) value_histograms; - /* 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; - /* For function.c. */ /* Points to the FUNCTION_DECL of this function. */ Index: trans-mem.c =================================================================== --- trans-mem.c (revision 181028) +++ trans-mem.c (working copy) @@ -172,9 +172,13 @@ get_attrs_for (const_tree x) bool is_tm_pure (const_tree x) { - tree attrs = get_attrs_for (x); - if (attrs) - return lookup_attribute ("transaction_pure", attrs) != NULL; + if (flag_tm) + { + tree attrs = get_attrs_for (x); + if (attrs) + return lookup_attribute ("transaction_pure", attrs) != NULL; + return false; + } return false; } @@ -205,16 +209,17 @@ is_tm_irrevocable (tree x) bool is_tm_safe (const_tree x) { - tree attrs = get_attrs_for (x); - - if (attrs) + if (flag_tm) { - if (lookup_attribute ("transaction_safe", attrs)) - return true; - if (lookup_attribute ("transaction_may_cancel_outer", attrs)) - return true; + tree attrs = get_attrs_for (x); + if (attrs) + { + if (lookup_attribute ("transaction_safe", attrs)) + return true; + if (lookup_attribute ("transaction_may_cancel_outer", attrs)) + return true; + } } - return false; } @@ -270,22 +275,6 @@ is_tm_may_cancel_outer (tree x) return false; } -/* Return true if STMT may alter control flow via a transactional edge. */ - -bool -is_transactional_stmt (const_gimple stmt) -{ - switch (gimple_code (stmt)) - { - case GIMPLE_CALL: - return (gimple_call_flags (stmt) & ECF_TM_OPS) != 0; - case GIMPLE_TRANSACTION: - return true; - default: - return false; - } -} - /* Return true for built in functions that "end" a transaction. */ bool @@ -2470,13 +2459,13 @@ make_tm_edge (gimple stmt, basic_block b void **slot; struct tm_restart_node *n, dummy; - if (cfun->tm_restart == NULL) - cfun->tm_restart = htab_create_ggc (31, struct_ptr_hash, - struct_ptr_eq, ggc_free); + if (cfun->gimple_df->tm_restart == NULL) + cfun->gimple_df->tm_restart = htab_create_ggc (31, struct_ptr_hash, + struct_ptr_eq, ggc_free); dummy.stmt = stmt; dummy.label_or_list = gimple_block_label (region->entry_block); - slot = htab_find_slot (cfun->tm_restart, &dummy, INSERT); + slot = htab_find_slot (cfun->gimple_df->tm_restart, &dummy, INSERT); n = (struct tm_restart_node *) *slot; if (n == NULL) { Index: cfgexpand.c =================================================================== --- cfgexpand.c (revision 181028) +++ cfgexpand.c (working copy) @@ -2097,13 +2097,13 @@ expand_gimple_stmt (gimple stmt) } /* Mark all calls that can have a transaction restart. */ - if (cfun->tm_restart && is_gimple_call (stmt)) + if (cfun->gimple_df->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); + slot = htab_find_slot (cfun->gimple_df->tm_restart, &dummy, NO_INSERT); if (slot) { struct tm_restart_node *n = (struct tm_restart_node *) *slot; @@ -4483,7 +4483,11 @@ gimple_expand_cfg (void) naked_return_label = NULL; /* After expanding, the tm_restart map is no longer needed. */ - cfun->tm_restart = NULL; + if (cfun->gimple_df->tm_restart) + { + htab_delete (cfun->gimple_df->tm_restart); + cfun->gimple_df->tm_restart = NULL; + } /* Tag the blocks with a depth number so that change_scope can find the common parent easily. */ Index: tree-flow.h =================================================================== --- tree-flow.h (revision 181028) +++ tree-flow.h (working copy) @@ -33,6 +33,14 @@ along with GCC; see the file COPYING3. #include "tree-ssa-alias.h" +/* This structure is used to map a gimple statement to a label, + or list of labels to represent transaction restart. */ + +struct GTY(()) tm_restart_node { + gimple stmt; + tree label_or_list; +}; + /* Gimple dataflow datastructure. All publicly available fields shall have gimple_ accessor defined in tree-flow-inline.h, all publicly modifiable fields should have gimple_set accessor. */ @@ -80,6 +88,10 @@ struct GTY(()) gimple_df { unsigned int ipa_pta : 1; struct ssa_operands ssa_operands; + + /* 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; }; /* Accessors for internal use only. Generic code should use abstraction @@ -778,9 +790,6 @@ extern bool maybe_duplicate_eh_stmt (gim extern bool verify_eh_edges (gimple); extern bool verify_eh_dispatch_edge (gimple); -/* In gtm-low.c */ -extern bool is_transactional_stmt (const_gimple); - /* In tree-ssa-pre.c */ struct pre_expr_d; void add_to_value (unsigned int, struct pre_expr_d *); Index: tree-cfg.c =================================================================== --- tree-cfg.c (revision 181028) +++ tree-cfg.c (working copy) @@ -117,6 +117,7 @@ static int gimple_verify_flow_info (void static void gimple_make_forwarder_block (edge); static void gimple_cfg2vcg (FILE *); static gimple first_non_label_stmt (basic_block); +static bool verify_gimple_transaction (gimple); /* Flowgraph optimization and cleanup. */ static void gimple_merge_blocks (basic_block, basic_block); @@ -4098,18 +4099,6 @@ verify_gimple_switch (gimple stmt) return false; } -/* 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; - return false; -} - /* Verify a gimple debug statement STMT. Returns true if anything is wrong. */ @@ -4339,7 +4328,7 @@ verify_gimple_in_seq_2 (gimple_seq stmts break; case GIMPLE_TRANSACTION: - err |= verify_gimple_in_seq_2 (gimple_transaction_body (stmt)); + err |= verify_gimple_transaction (stmt); break; default: @@ -4355,6 +4344,18 @@ verify_gimple_in_seq_2 (gimple_seq stmts return err; } +/* 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; + return verify_gimple_in_seq_2 (gimple_transaction_body (stmt)); +} + /* Verify the GIMPLE statements inside the statement list STMTS. */ @@ -5122,9 +5123,10 @@ gimple_redirect_edge_and_branch (edge e, 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) + /* ??? 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)); + */ break; default: