From patchwork Thu Dec 19 16:04:48 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 303603 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 87ACF2C009E for ; Fri, 20 Dec 2013 03:05:00 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:content-type; q= dns; s=default; b=yZbwOVmbz7mbtXtHgHIDdEWrKUrP3D3p8G/4mndp4jcQeJ zU9Xwnu2x2T69k965nIPjwGIqMSiHD1Z4zHgjlhOTPDGtgnCIQ04xbYhXRTxtIkx BvH4DsT4F5FEz8uB5TNOdqnowaKWUnovhpaMg7ly5oAyzAhJkiXe1QRNDwxPM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:content-type; s= default; bh=uZ9wMSILW24affSRld1WfimaO/w=; b=WGNbLsZ3PirQ3g9ji8tl hnhGEr5/sHsL2j0Jz9/xzvToUv1CL1WcY2o0N+RZZqKG3eK1x/VuKk6hxVCY0J9Z hsWBW+kfMdJFKOuiwSw66BLUdgSMRGwE+dNgRXCxPAjXS+IN35ozeww7uoUB0Ucl yETIPdsWDnCTRhbrdGGIfO0= Received: (qmail 8141 invoked by alias); 19 Dec 2013 16:04:53 -0000 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 Received: (qmail 8129 invoked by uid 89); 19 Dec 2013 16:04:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Dec 2013 16:04:51 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBJG4n1x002105 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 19 Dec 2013 11:04:50 -0500 Received: from reynosa.quesejoda.com (vpn-52-230.rdu2.redhat.com [10.10.52.230]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rBJG4mgL024277; Thu, 19 Dec 2013 11:04:49 -0500 Message-ID: <52B31920.9080501@redhat.com> Date: Thu, 19 Dec 2013 08:04:48 -0800 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: gcc-patches , Richard Henderson Subject: [patch] PR56572 flatten unnecessary nested transactions after inlining As discussed in the PR, we already remove nested transactions in the tmlower stage, but inlining may add more nested transactions later. The problem with removing these nested transactions after proper IPA, is that we'd either have to add another IPA pass later or remove the problematic transactions much later in the .tmmark stage. Either of these places will be after we have added the uninstrumented code path, so things look a little messy CFG-wise (not impossible to do, but messy). What I've done in this patch is to fix the inliner cost model so that the early inliner sees and inlines the transaction early on. This way, .tmipa can see the inlined nested transaction before it adds the uninstrumented code path, making everything simpler. Then it's just a matter of removing the GIMPLE_TRANSACTION and associated commit. I'm still unsure whether the IPA inliner (not the early inliner) will add other nested transactions, so we may have to do everything in .tmmark and handle the dual code paths :(. Either way, this is a start. Also, is there anything I should check here, or is checking for GTMA_HAVE_ABORT enough? + /* An inner transactions which has no explicit aborts can be + folded into its outer transaction. */ + unsigned int sub = gimple_transaction_subcode (region->transaction_stmt); + if (outer && !(sub & GTMA_HAVE_ABORT)) Tested on x86-64 Linux. How does this look? commit 460af022f8dcc5fb67410a8cd6a33d2be13819b9 Author: Aldy Hernandez Date: Wed Dec 18 09:53:54 2013 -0800 PR tree-optimization/56572 * tree-inline.c (init_inline_once): Adjust .tm_cost size. * trans-mem.c (remove_transaction): New. (flatten_nested_transactions_1): New. (flatten_nested_transactions): New. (ipa_tm_execute): Call flatten_nested_transactions. diff --git a/gcc/testsuite/gcc.dg/tm/pr56572.c b/gcc/testsuite/gcc.dg/tm/pr56572.c new file mode 100644 index 0000000..0b43569 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tm/pr56572.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-options "-fgnu-tm -fdump-ipa-tmipa -O2" } */ + +/* Test that nested unnecessary transactions are properly folded. */ + +int a; + +static inline void f() { + __transaction_atomic { + ++a; + } +} + +void g() { + __transaction_atomic { + f(); + } +} + +/* { dg-final { scan-ipa-dump-times "__transaction_atomic" 1 "tmipa" } } */ +/* { dg-final { cleanup-ipa-dump "tmipa" } } */ diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c index ba34488..5e9e2ab 100644 --- a/gcc/trans-mem.c +++ b/gcc/trans-mem.c @@ -2956,6 +2956,120 @@ propagate_tm_flags_out (struct tm_region *region) propagate_tm_flags_out (region->next); } +/* Callback for expand_regions. + + Given a transaction in REGION, remove the GIMPLE_TRANSACTION and + its corresponding BUILT_IN_TM_COMMIT*. The tm_region itself is + removed by the caller. */ + +static void * +remove_transaction (struct tm_region *region, void *data ATTRIBUTE_UNUSED) +{ + // Remove the transaction. + if (dump_file) + fprintf (dump_file, "Folding unnecessary inner transaction in BB%d\n", + gimple_bb (region->transaction_stmt)->index); + gimple_stmt_iterator gsi = gsi_for_stmt (region->transaction_stmt); + gsi_remove (&gsi, true); + + // Find the corresponding BUILT_IN_TM_COMMIT* and remove it as well. + if (region->exit_blocks) + { + bitmap_iterator bi; + unsigned i; + + EXECUTE_IF_SET_IN_BITMAP (region->exit_blocks, 0, i, bi) + { + gimple_stmt_iterator gsi + = gsi_last_bb (BASIC_BLOCK_FOR_FN (cfun, i)); + gimple stmt = gsi_stmt (gsi); + if (gimple_code (stmt) != GIMPLE_CALL) + continue; + tree fndecl = gimple_call_fndecl (stmt); + if (fndecl + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL + && (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_TM_COMMIT + || DECL_FUNCTION_CODE (fndecl) == BUILT_IN_TM_COMMIT_EH)) + { + tree vdef = gimple_vdef (stmt); + tree vuse = gimple_vuse (stmt); + if (vdef && vuse) + replace_uses_by (vdef, vuse); + gsi_remove (&gsi, true); + } + } + } + return NULL; +} + +/* A helper function for flatten_nested_transactions. + + Go through all the transactional regions, starting at TOP, and + remove any nested transactions that are redundant, removing both + the start/end of a transaction as well as removing the region + from the tm_region list. + + OUTER is TOP's outer transaction and is set to null if TOP is the + outermost transaction. + + Returns FALSE if the region passed in TOP should be removed from + the list of regions by the caller. */ +static bool +flatten_nested_transactions_1 (struct tm_region *top, struct tm_region *outer) +{ + /* We could fold this traversal into ipa_tm_scan_calls_transaction, + and only traverse all_tm_regions once, but the nested + transactions we handle below would complicate things. Besides, + there aren't liable to be many TM regions in a function to make + this extra iteration so unprofitable. + + ?? We could even only call flatten_nested_transactions from + within ipa_tm_scan_calls_transaction only if we have any inner + transactions. Hmmm, probably not worth it. */ + struct tm_region *prev = NULL; + for (struct tm_region *region = top; region; region = region->next) + { + if (region->inner + && !flatten_nested_transactions_1 (region->inner, region)) + region->inner = NULL; + + /* An inner transactions which has no explicit aborts can be + folded into its outer transaction. */ + unsigned int sub = gimple_transaction_subcode (region->transaction_stmt); + if (outer && !(sub & GTMA_HAVE_ABORT)) + { + expand_regions (region, remove_transaction, NULL, false); + + /* Remove region from the list if its anything but the first + item, otherwise make the caller remove the first + item. */ + if (prev == NULL) + return false; + else + prev->next = region->next; + } + prev = region; + } + return true; +} + +/* Go through all the transactional regions and remove any nested + transactions that are redundant. This is done by removing the + GIMPLE_TRANSACTION + BUILT_IN_TM_COMMIT* pairs and by removing the + transaction region from the all_tm_regions list. + + Note: We already did this in the tmlower pass (lower_transaction), + but inlining may have added more nested transactions that are now + candidates for removal. */ + +static void +flatten_nested_transactions () +{ + if (!flatten_nested_transactions_1 (all_tm_regions, NULL)) + all_tm_regions = NULL; +} + + /* Entry point to the MARK phase of TM expansion. Here we replace transactional memory statements with calls to builtins, and function calls with their transactional clones (if available). But we don't @@ -5358,6 +5472,8 @@ ipa_tm_execute (void) { d = get_cg_data (&node, true); + flatten_nested_transactions (); + /* Scan for calls that are in each transaction, and generate the uninstrumented code path. */ ipa_tm_scan_calls_transaction (d, &tm_callees); diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 96a4805..6c6ef41 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3972,7 +3972,7 @@ init_inline_once (void) eni_size_weights.target_builtin_call_cost = 1; eni_size_weights.div_mod_cost = 1; eni_size_weights.omp_cost = 40; - eni_size_weights.tm_cost = 10; + eni_size_weights.tm_cost = 2; eni_size_weights.time_based = false; eni_size_weights.return_cost = 1;