From patchwork Thu Nov 29 01:05:09 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 202631 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 53EF12C0087 for ; Thu, 29 Nov 2012 12:05:29 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1354755931; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:Subject: Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:Sender:Delivered-To; bh=YSEhihk IbYz6JbFKx3l9X9B1FwY=; b=T1WReObdLT0RjNF4CPWOOTtj/sgeUfgHfy4b2Ak PIo6AqLUnpaMbPr//O7ycuUN9ZoEm3EYlEhc8cpZerc8L8d+csxGufMiet93p4IJ Dz0BfchzpAKp29SmH0n1fSDDQvJ41PcGoLLjiruwy2rsBB+OryP0dKAY/qiVaRv3 9NWE= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:Subject:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=HLwmTLJa9tXqXlXIAJ1xWXK7baXDboFvyImhePspfyAL1Cjx4v2mHx3TyJ1f8X jaEH5nUhmLxh0UioQy4UfFRiWhhuEyBsSukJhzkZJokq3USr9iLtrLEO45G+44N+ Q020uE3qlJWzn+i04i+f42BEH5jlmeQ835cNmIfy7+utU=; Received: (qmail 25739 invoked by alias); 29 Nov 2012 01:05:24 -0000 Received: (qmail 25726 invoked by uid 22791); 29 Nov 2012 01:05:23 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, RCVD_IN_DNSWL_HI, RCVD_IN_HOSTKARMA_W, RP_MATCHES_RCVD, SPF_HELO_PASS 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; Thu, 29 Nov 2012 01:05:16 +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 qAT15FLO023123 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 28 Nov 2012 20:05:15 -0500 Received: from houston.quesejoda.com (vpn-11-200.rdu.redhat.com [10.11.11.200]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qAT15AS9032027; Wed, 28 Nov 2012 20:05:14 -0500 Message-ID: <50B6B4C5.9050402@redhat.com> Date: Wed, 28 Nov 2012 19:05:09 -0600 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121016 Thunderbird/16.0.1 MIME-Version: 1.0 To: Richard Henderson , gcc-patches Subject: [patch] PR middle-end/55401: uninstrument relevant path in TM clone 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 In this testcase: __attribute__((transaction_callable)) void q1() { ringo=666; __transaction_atomic { george=999; } } ...the clone for q1() ends up with instrumented code on both pathways: : _12 = tm_state.5_11 & 2; if (_12 != 0) goto ; else goto ; : _13 = 999; __builtin__ITM_WU4 (&george, _13); __builtin__ITM_commitTransaction (); goto ; : _15 = 999; __builtin__ITM_WU4 (&george, _15); __builtin__ITM_commitTransaction (); The reason this happens is because collect_bb2reg() follows the uninstrumented edges while traversing a transaction. In the attached patch, I added yet another flag (*sigh*) to get_tm_region_blocks() specifying whether we should skip or include the uninstrumented blocks while accumulating basic blocks. To assuage the grief, I make the new argument optional. With the attached path, we generate this: : _12 = tm_state.5_11 & 2; if (_12 != 0) goto ; else goto ; : george = 999; __builtin__ITM_commitTransaction (); goto ; : _13 = 999; __builtin__ITM_WU4 (&george, _13); __builtin__ITM_commitTransaction (); ...respecting the uninstrumented flag. BTW, collect_bb2reg() is also called from execute_tm_edges() (via get_bb_regions_instrumented), hence the additional callback data to collect_bb2reg(). From TMMARK we wish to exclude the instrumented path, but in TMEDGE we should include it, so we can properly add cancel edges from the uninstrumented code path. [I could abstract get_bb_regions_instrumented into two versions, an instrumented one (as the name suggests) and an uninstrumented one. Perhaps this would be clearer]. Blah. OK for trunk? commit d0d101b53552336f6133cced41aff746319cd074 Author: Aldy Hernandez Date: Wed Nov 28 18:17:33 2012 -0600 PR middle-end/55401 * trans-mem.c (get_tm_region_blocks): Exclude uninstrumented blocks from vector if requested. (collect_bb2reg): Pass new argument to get_tm_region_blocks. (get_bb_regions_instrumented): Add INCLUDE_UNINSTRUMENTED_P argument, and pass it to expand_regions. (execute_tm_mark): Pass new argument to get_bb_regions_instrumented. (execute_tm_edges): Same. diff --git a/gcc/testsuite/gcc.dg/tm/pr55401.c b/gcc/testsuite/gcc.dg/tm/pr55401.c new file mode 100644 index 0000000..1ac7d97 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tm/pr55401.c @@ -0,0 +1,22 @@ +/* { dg-do compile } */ +/* { dg-options "-fgnu-tm -O0 -fdump-tree-optimized" } */ + +int george; +int ringo; + +__attribute__((transaction_callable)) +void foo() +{ + ringo=666; + __transaction_atomic { + george=999; + } +} + +/* There should only be 2 instrumented writes to GEORGE: one in FOO, + and one in the transactional clone to FOO. There should NOT be + more than one instrumented write to GEORGE in the clone of + FOO. */ +/* { dg-final { scan-tree-dump-times "ITM_WU\[0-9\] \\(&george," 2 "optimized" } } */ + +/* { dg-final { cleanup-tree-dump "optimized" } } */ diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c index 0a428fe..8762ad3 100644 --- a/gcc/trans-mem.c +++ b/gcc/trans-mem.c @@ -2394,14 +2394,19 @@ expand_block_tm (struct tm_region *region, basic_block bb) /* Return the list of basic-blocks in REGION. STOP_AT_IRREVOCABLE_P is true if caller is uninterested in blocks - following a TM_IRREVOCABLE call. */ + following a TM_IRREVOCABLE call. + + INCLUDE_UNINSTRUMENTED_P is TRUE if we should include the + uninstrumented code path blocks in the list of basic blocks + returned, false otherwise. */ static vec get_tm_region_blocks (basic_block entry_block, bitmap exit_blocks, bitmap irr_blocks, bitmap all_region_blocks, - bool stop_at_irrevocable_p) + bool stop_at_irrevocable_p, + bool include_uninstrumented_p = true) { vec bbs = vNULL; unsigned i; @@ -2427,7 +2432,9 @@ get_tm_region_blocks (basic_block entry_block, continue; FOR_EACH_EDGE (e, ei, bb->succs) - if (!bitmap_bit_p (visited_blocks, e->dest->index)) + if ((include_uninstrumented_p + || !(e->flags & EDGE_TM_UNINSTRUMENTED)) + && !bitmap_bit_p (visited_blocks, e->dest->index)) { bitmap_set_bit (visited_blocks, e->dest->index); bbs.safe_push (e->dest); @@ -2442,11 +2449,19 @@ get_tm_region_blocks (basic_block entry_block, return bbs; } +// Callback data for collect_bb2reg. +struct bb2reg_stuff +{ + vec *bb2reg; + bool include_uninstrumented_p; +}; + // Callback for expand_regions, collect innermost region data for each bb. static void * collect_bb2reg (struct tm_region *region, void *data) { - vec *bb2reg = (vec *) data; + struct bb2reg_stuff *stuff = (struct bb2reg_stuff *)data; + vec *bb2reg = stuff->bb2reg; vec queue; unsigned int i; basic_block bb; @@ -2455,7 +2470,8 @@ collect_bb2reg (struct tm_region *region, void *data) region->exit_blocks, region->irr_blocks, NULL, - /*stop_at_irr_p=*/true); + /*stop_at_irr_p=*/true, + stuff->include_uninstrumented_p); // We expect expand_region to perform a post-order traversal of the region // tree. Therefore the last region seen for any bb is the innermost. @@ -2468,7 +2484,8 @@ collect_bb2reg (struct tm_region *region, void *data) // Returns a vector, indexed by BB->INDEX, of the innermost tm_region to // which a basic block belongs. Note that we only consider the instrumented -// code paths for the region; the uninstrumented code paths are ignored. +// code paths for the region; the uninstrumented code paths are ignored if +// INCLUDE_UNINSTRUMENTED_P is false. // // ??? This data is very similar to the bb_regions array that is collected // during tm_region_init. Or, rather, this data is similar to what could @@ -2489,14 +2506,18 @@ collect_bb2reg (struct tm_region *region, void *data) // only known instance of this block sharing. static vec -get_bb_regions_instrumented (bool traverse_clones) +get_bb_regions_instrumented (bool traverse_clones, + bool include_uninstrumented_p) { unsigned n = last_basic_block; + struct bb2reg_stuff stuff; vec ret; ret.create (n); ret.safe_grow_cleared (n); - expand_regions (all_tm_regions, collect_bb2reg, &ret, traverse_clones); + stuff.bb2reg = &ret; + stuff.include_uninstrumented_p = include_uninstrumented_p; + expand_regions (all_tm_regions, collect_bb2reg, &stuff, traverse_clones); return ret; } @@ -2830,7 +2851,8 @@ execute_tm_mark (void) tm_log_init (); vec bb_regions - = get_bb_regions_instrumented (/*traverse_clones=*/true); + = get_bb_regions_instrumented (/*traverse_clones=*/true, + /*include_uninstrumented_p=*/false); struct tm_region *r; unsigned i; @@ -3004,7 +3026,8 @@ static unsigned int execute_tm_edges (void) { vec bb_regions - = get_bb_regions_instrumented (/*traverse_clones=*/false); + = get_bb_regions_instrumented (/*traverse_clones=*/false, + /*include_uninstrumented_p=*/true); struct tm_region *r; unsigned i;