From patchwork Sun Nov 6 21:24:54 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 123968 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 9C3BE1007D2 for ; Mon, 7 Nov 2011 08:25:17 +1100 (EST) Received: (qmail 14753 invoked by alias); 6 Nov 2011 21:25:15 -0000 Received: (qmail 14731 invoked by uid 22791); 6 Nov 2011 21:25:12 -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 21:24:57 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pA6LOuW4006348 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 6 Nov 2011 16:24:56 -0500 Received: from austin.quesejoda.com (vpn-11-235.rdu.redhat.com [10.11.11.235]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id pA6LOs5H023377; Sun, 6 Nov 2011 16:24:55 -0500 Message-ID: <4EB6FB26.10505@redhat.com> Date: Sun, 06 Nov 2011 13:24:54 -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 Henderson CC: Richard Guenther , gcc-patches Subject: Re: [patch] 19/n: trans-mem: middle end/misc patches (LAST PATCH) References: <4EB2EC3F.6000908@redhat.com> <4EB6D797.4070309@redhat.com> <4EB6EC02.8010907@redhat.com> In-Reply-To: <4EB6EC02.8010907@redhat.com> 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 >> Actually, the table organization is irrelevant, because upon >> registering of the table in the runtime, we qsort the entire thing. > > False. You get the equivalent of bootstrap comparison mismatches. > If we actually used tm during the bootstrap. > > The simplest thing to do is to change the hash this table uses. > E.g. use the DECL_UID right from the start, rather than the pointer. Argh, will fix in a followup patch. >>>> - 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. > > Really? I have no idea what this change achieves. > I actually wonder if this is a merge error. I won't complain :). I have reverted the original patch and am including it in the final (attched) version I will commit. >>>> + 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? > > I think that the comment is wrong. We need that edge, and the label updated until pass_tm_edges, at which point the GIMPLE_TRANSACTION itself goes away. Thus that label is live throughout the live of the GIMPLE_TRANSACTION node. > > Delete that ??? comment instead. Done. > Patch is otherwise ok. Attched is the final revision of the patch. I will commit once tests finish. thank you. * tree-cfg.c (verify_gimple_transaction): Verify body. Move down. (verify_gimple_in_seq_2): Verify the label in a GIMPLE_TRANSACTION. (cleanup_dead_labels): Remove GIMPLE_GOTO hiccup from merge. * 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); @@ -1262,27 +1263,13 @@ cleanup_dead_labels (void) /* We have to handle gotos until they're removed, and we don't remove them until after we've created the CFG edges. */ case GIMPLE_GOTO: - if (!computed_goto_p (stmt)) + if (!computed_goto_p (stmt)) { - label = gimple_goto_dest (stmt); - new_label = main_block_label (label); - if (new_label != label) - gimple_goto_set_dest (stmt, new_label); + tree new_dest = main_block_label (gimple_goto_dest (stmt)); + gimple_goto_set_dest (stmt, new_dest); } break; - case GIMPLE_TRANSACTION: - { - tree label = gimple_transaction_label (stmt); - if (label) - { - tree new_label = main_block_label (label); - if (new_label != label) - gimple_transaction_set_label (stmt, new_label); - } - } - break; - default: break; } @@ -4098,18 +4085,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 +4314,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 +4330,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,7 +5109,6 @@ 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) gimple_transaction_set_label (stmt, gimple_block_label (dest)); break;