From patchwork Fri Nov 16 21:28:01 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 199734 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 A62CA2C0089 for ; Sat, 17 Nov 2012 08:28:17 +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=1353706099; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject: Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:Sender:Delivered-To; bh=Y1TJWSY QwSAvolV5C/eTUjyfel4=; b=VfouiEAtOrnIuQmp2hGF7PVtHnFKn+xkLPOeyna eNDDanMacdz2F2jodHmS8aSQJnFmvoyfjNO+VmXUPzEJMVN+Yquc3y7Iuh1cn5vX JeGZEkotnHjSFd9BAWu0Jm3eZHHdKkF60kzTgtqgDA+BhTFFyblN59/nkOCqZHAa VF6U= 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:CC:Subject:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=lq9IJyE+inl3zN3sT9Y3fK50Vrviz6XPxwE3mSGCnqVmAo2fnmIWphGlKI1GLD XE+S7d43nqq7I7sLMnBu4f0fb0OccnDNsxObUsCXaXPO1xthpCLwXz0OJv4i5LLr sOxVEgwpVzee8i3v226kBWM4QfFDpoqRJeOpHUll32KOg=; Received: (qmail 14455 invoked by alias); 16 Nov 2012 21:28:13 -0000 Received: (qmail 14447 invoked by uid 22791); 16 Nov 2012 21:28:12 -0000 X-SWARE-Spam-Status: No, hits=-6.4 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; Fri, 16 Nov 2012 21:28:04 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qAGLS3W7010404 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 16 Nov 2012 16:28:03 -0500 Received: from houston.quesejoda.com (vpn-9-69.rdu.redhat.com [10.11.9.69]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qAGLS2xf015187; Fri, 16 Nov 2012 16:28:02 -0500 Message-ID: <50A6AFE1.9060103@redhat.com> Date: Fri, 16 Nov 2012 15:28:01 -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 CC: gcc-patches Subject: [patch] instrument clones 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 The included small change to g++.dg/tm/pr51516.C fixes the remaining TM regression. With two code paths, there are two instances of the constructor (a clone and an uninstrumented version), so that part of the test is no longer relevant. However... in fixing this, I noticed that for some odd reason we are not instrumenting clones at all. I have no idea, how we missed this, but if you look at the (currently) generated code for: __attribute__((transaction_callable)) void cloneme() { foo = 666; } ...you will notice that the clone version has no instrumentation. This is because the iteration with get_bb_regions_uninstrumented() exits if there are no exit_blocks, which is always the case for TM clones. I added a new parameter so we can force traversal of the clone if we are looking at a clone. But then this is problematic if the clone has inline assembly because collect_bb2reg() does not stop at irrevocable blocks. So if we indiscriminately scan clones, we plow right through irrevocable blocks and try to incorrectly instrument them. Fixed with yet another parameter. With this patch we have no TM regressions whatsoever, and as a bonus we are instrumenting clones :-). OK? commit 92723b5bb9d6791b1f3466e5106db13f143da2ca Author: Aldy Hernandez Date: Fri Nov 16 15:12:47 2012 -0600 * trans-mem (collect_bb2reg): Stop scanning at irrevocable * blocks. (get_bb_regions_instrumented): Add new traverse_clone argument and use it. (expand_regions_1): Same. (expand_region): Same. (execute_tm_mark): Pass new argument to expand_regions. (expand_block_edges): Pass new argument to get_bb_regions_instrumented. testsuite/ * g++.dg/tm/pr51516.C: Adjust for uninstrumented code path. * gcc.dg/tm/clone-1.c: New test. diff --git a/gcc/testsuite/g++.dg/tm/pr51516.C b/gcc/testsuite/g++.dg/tm/pr51516.C index c13ae47..4e91006 100644 --- a/gcc/testsuite/g++.dg/tm/pr51516.C +++ b/gcc/testsuite/g++.dg/tm/pr51516.C @@ -18,5 +18,4 @@ int main() } /* { dg-final { scan-assembler-not "_ITM_getTMCloneOrIrrevocable" } } */ -/* { dg-final { scan-tree-dump-times ";; Function C::C" 1 "optimized" } } */ /* { dg-final { cleanup-tree-dump "optimized" } } */ diff --git a/gcc/testsuite/gcc.dg/tm/clone-1.c b/gcc/testsuite/gcc.dg/tm/clone-1.c new file mode 100644 index 0000000..4050add --- /dev/null +++ b/gcc/testsuite/gcc.dg/tm/clone-1.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-fgnu-tm -fdump-tree-tmmark" } */ + +int foo; + +__attribute__((transaction_callable)) +void cloneme() +{ + foo = 666; +} + +/* { dg-final { scan-tree-dump-times "ITM_WU.*foo" 1 "tmmark" } } */ +/* { dg-final { cleanup-tree-dump "tmmark" } } */ diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c index a7b4a9c..14d0ca9 100644 --- a/gcc/trans-mem.c +++ b/gcc/trans-mem.c @@ -138,7 +138,7 @@ static void *expand_regions (struct tm_region *, void *(*callback)(struct tm_region *, void *), - void *); + void *, bool); /* Return the attributes we want to examine for X, or NULL if it's not @@ -2457,7 +2457,7 @@ collect_bb2reg (struct tm_region *region, void *data) region->exit_blocks, region->irr_blocks, NULL, - /*stop_at_irr_p=*/false); + /*stop_at_irr_p=*/true); // 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. @@ -2491,14 +2491,14 @@ collect_bb2reg (struct tm_region *region, void *data) // only known instance of this block sharing. static VEC(tm_region_p, heap) * -get_bb_regions_instrumented (void) +get_bb_regions_instrumented (bool traverse_clones) { unsigned n = last_basic_block; VEC(tm_region_p, heap) *ret; ret = VEC_alloc (tm_region_p, heap, n); VEC_safe_grow_cleared (tm_region_p, heap, ret, n); - expand_regions (all_tm_regions, collect_bb2reg, ret); + expand_regions (all_tm_regions, collect_bb2reg, ret, traverse_clones); return ret; } @@ -2826,11 +2826,13 @@ execute_tm_mark (void) { pending_edge_inserts_p = false; - expand_regions (all_tm_regions, generate_tm_state, NULL); + expand_regions (all_tm_regions, generate_tm_state, NULL, + /*traverse_clones=*/true); tm_log_init (); - VEC(tm_region_p, heap) *bb_regions = get_bb_regions_instrumented (); + VEC(tm_region_p, heap) *bb_regions + = get_bb_regions_instrumented (/*traverse_clones=*/true); struct tm_region *r; unsigned i; @@ -2844,7 +2846,8 @@ execute_tm_mark (void) propagate_tm_flags_out (all_tm_regions); // Expand GIMPLE_TRANSACTIONs into calls into the runtime. - expand_regions (all_tm_regions, expand_transaction, NULL); + expand_regions (all_tm_regions, expand_transaction, NULL, + /*traverse_clones=*/false); tm_log_emit (); tm_log_delete (); @@ -3000,7 +3003,8 @@ expand_block_edges (struct tm_region *const region, basic_block bb) static unsigned int execute_tm_edges (void) { - VEC(tm_region_p, heap) *bb_regions = get_bb_regions_instrumented (); + VEC(tm_region_p, heap) *bb_regions + = get_bb_regions_instrumented (/*traverse_clones=*/false); struct tm_region *r; unsigned i; @@ -3044,15 +3048,18 @@ struct gimple_opt_pass pass_tm_edges = /* Helper function for expand_regions. Expand REGION and recurse to the inner region. Call CALLBACK on each region. CALLBACK returns NULL to continue the traversal, otherwise a non-null value which - this function will return as well. */ + this function will return as well. TRAVERSE_CLONES is true if we + should traverse transactional clones. */ static void * expand_regions_1 (struct tm_region *region, void *(*callback)(struct tm_region *, void *), - void *data) + void *data, + bool traverse_clones) { void *retval = NULL; - if (region->exit_blocks) + if (region->exit_blocks + || (traverse_clones && decl_is_tm_clone (current_function_decl))) { retval = callback (region, data); if (retval) @@ -3060,7 +3067,7 @@ expand_regions_1 (struct tm_region *region, } if (region->inner) { - retval = expand_regions (region->inner, callback, data); + retval = expand_regions (region->inner, callback, data, traverse_clones); if (retval) return retval; } @@ -3070,17 +3077,19 @@ expand_regions_1 (struct tm_region *region, /* Traverse the regions enclosed and including REGION. Execute CALLBACK for each region, passing DATA. CALLBACK returns NULL to continue the traversal, otherwise a non-null value which this - function will return as well. */ + function will return as well. TRAVERSE_CLONES is true if we should + traverse transactional clones. */ static void * expand_regions (struct tm_region *region, void *(*callback)(struct tm_region *, void *), - void *data) + void *data, + bool traverse_clones) { void *retval = NULL; while (region) { - retval = expand_regions_1 (region, callback, data); + retval = expand_regions_1 (region, callback, data, traverse_clones); if (retval) return retval; region = region->next;