From patchwork Tue Feb 28 17:44:23 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 143515 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 47CCCB6EE7 for ; Wed, 29 Feb 2012 04:44:47 +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=1331055888; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject: References:In-Reply-To:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=tv4kRwWUl+2VL6gnobBFHVhNJBg=; b=OjTpP0JXTwFP74p 2ISQeHD09u8xsqLzehav8W3ar9vthPg4wF4wxlJ2B7KU/Mf3qYSCWzbMntojjQ+Z 5a5V/VxbcxQCGc1InynoJX6KKY/1hhynLdzplXdSD/ItmVb6BC3T4LBhR039w566 E1vPQ34zY05rnacYoN7xWb8vrJ/A= 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:References:In-Reply-To:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=XvvkBDaYPysXGYVfMdt8Hk8+cv1nlABHWZvW/hkVPm96ZakbgDMDtkjJKibm71 P3uRsCiFXNtZVFZSSFDHEqyOUZ2Vl0OOx+05pIwrUjlybg+vce2YXJaEN9I4qPSe bduuumlrSm4UZUIrMwcspuzDANCPT3h0DV2QmhVoaa/dM=; Received: (qmail 772 invoked by alias); 28 Feb 2012 17:44:42 -0000 Received: (qmail 761 invoked by uid 22791); 28 Feb 2012 17:44:40 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, SPF_HELO_PASS, TW_TM, T_RP_MATCHES_RCVD 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; Tue, 28 Feb 2012 17:44:24 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1SHiOq2000802 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 28 Feb 2012 12:44:24 -0500 Received: from houston.quesejoda.com (vpn-237-139.phx2.redhat.com [10.3.237.139]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q1SHiNML019575; Tue, 28 Feb 2012 12:44:23 -0500 Message-ID: <4F4D1277.9080206@redhat.com> Date: Tue, 28 Feb 2012 11:44:23 -0600 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120216 Thunderbird/10.0.1 MIME-Version: 1.0 To: Richard Henderson CC: Richard Guenther , Torvald Riegel , gcc-patches Subject: Re: [PR51752] publication safety violations in loop invariant motion pass References: <4F46833C.2090808@redhat.com> <4F46AB9D.7050407@redhat.com> <1330089023.2986.3085.camel@triegel.csb> <4F4BADD1.1090407@redhat.com> <4F4D0965.8020108@redhat.com> In-Reply-To: <4F4D0965.8020108@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 On 02/28/12 11:05, Richard Henderson wrote: > On 02/27/12 08:22, Aldy Hernandez wrote: >>> transform by making transaction load/store stmts behave the same as >>> potentially trapping stmts (thus, only optimize if the memory is accessed >>> unconditional somewhere else). That would work for PRE as well. >>> [easiest would be to make *_could_trap_p return true for memory ops >>> inside a transaction] >> >> Provided the gimple bit works, this seems reasonable, though quite a big hammer. But given that we are nearing a release, I would be in favor of it. >> >> Richard Henderson, what do you think? > > Well, hooking could_trap_p sounds like an easy solution. > > Gimple bits, on the other hand, are not. Keeping those up-to-date is > always a real pain. We have had several gimple bits in the history of > the TM code, and we've gotten rid of them all because they were too > invasive to maintain. > > OTOH, I have no better suggestion... Well, my solution wasn't pretty either. However, Mr. Guenther's solution involves recalculating the bits when needed, so at least they won't be out of date. Richards, what do y'all think of this approach? It is a big hammer as discussed, but keeps most of us happy for 4.7. Tested on x86-64 Linux. Aldy PR middle-end/51752 * gimple.h (gimple_in_transaction): New. (gimple_set_in_transaction): New. (struct gimple_statement_base): Add in_transaction field. * tree-ssa-loop-im.c: (movement_possibility): Restrict movement of transaction loads. (tree_ssa_lim_initialize): Compute transaction bits. * tree.h (compute_transaction_bits): Protoize. * trans-mem.c (tm_region_init): Use the heap to store BB auxilliary data. (compute_transaction_bits): New. Index: tree-ssa-loop-im.c =================================================================== --- tree-ssa-loop-im.c (revision 184271) +++ tree-ssa-loop-im.c (working copy) @@ -150,7 +150,7 @@ typedef struct mem_ref bitmap indep_ref; /* The set of memory references on that this reference is independent. */ - bitmap dep_ref; /* The complement of DEP_REF. */ + bitmap dep_ref; /* The complement of INDEP_REF. */ } *mem_ref_p; DEF_VEC_P(mem_ref_p); @@ -412,6 +412,26 @@ movement_possibility (gimple stmt) || gimple_could_trap_p (stmt)) return MOVE_PRESERVE_EXECUTION; + /* Non local loads in a transaction cannot be hoisted out. Well, + unless the load happens on every path out of the loop, but we + don't take this into account yet. */ + if (flag_tm + && gimple_in_transaction (stmt) + && gimple_assign_single_p (stmt)) + { + tree rhs = gimple_assign_rhs1 (stmt); + if (DECL_P (rhs) && is_global_var (rhs)) + { + if (dump_file) + { + fprintf (dump_file, "Cannot hoist conditional load of "); + print_generic_expr (dump_file, rhs, TDF_SLIM); + fprintf (dump_file, " because it is in a transaction.\n"); + } + return MOVE_IMPOSSIBLE; + } + } + return ret; } @@ -2387,6 +2407,9 @@ tree_ssa_lim_initialize (void) sbitmap_free (contains_call); lim_aux_data_map = pointer_map_create (); + + if (flag_tm) + compute_transaction_bits (); } /* Cleans up after the invariant motion pass. */ Index: testsuite/gcc.dg/tm/pub-safety-1.c =================================================================== --- testsuite/gcc.dg/tm/pub-safety-1.c (revision 0) +++ testsuite/gcc.dg/tm/pub-safety-1.c (revision 0) @@ -0,0 +1,24 @@ +/* { dg-do compile } */ +/* { dg-options "-fgnu-tm -O1 -fdump-tree-lim1" } */ + +/* Test that thread visible loads do not get hoisted out of loops if + the load would not have occurred on each path out of the loop. */ + +int x[10] = {0,0,0,0,0,0,0,0,0,0}; +int DATA_DATA = 0; + +void reader() +{ + int i; + __transaction_atomic + { + for (i = 0; i < 10; i++) + if (x[i]) + x[i] += DATA_DATA; + /* If we loaded DATA_DATA here, we could hoist it before the loop, + but since we don't... we can't. */ + } +} + +/* { dg-final { scan-tree-dump-times "Cannot hoist.*DATA_DATA because it is in a transaction" 1 "lim1" } } */ +/* { dg-final { cleanup-tree-dump "lim1" } } */ Index: trans-mem.c =================================================================== --- trans-mem.c (revision 184272) +++ trans-mem.c (working copy) @@ -1858,18 +1858,25 @@ tm_region_init (struct tm_region *region VEC(basic_block, heap) *queue = NULL; bitmap visited_blocks = BITMAP_ALLOC (NULL); struct tm_region *old_region; + struct tm_region **region_worklist; all_tm_regions = region; bb = single_succ (ENTRY_BLOCK_PTR); + /* We could store this information in bb->aux, but we may get called + through get_all_tm_blocks() from another pass that may be already + using bb->aux. */ + region_worklist = + (struct tm_region **) xcalloc (sizeof (struct tm_region *), + n_basic_blocks + NUM_FIXED_BLOCKS + 2); + VEC_safe_push (basic_block, heap, queue, bb); - gcc_assert (!bb->aux); /* FIXME: Remove me. */ - bb->aux = region; + region_worklist[bb->index] = region; do { bb = VEC_pop (basic_block, queue); - region = (struct tm_region *)bb->aux; - bb->aux = NULL; + region = region_worklist[bb->index]; + region_worklist[bb->index] = NULL; /* Record exit and irrevocable blocks. */ region = tm_region_init_1 (region, bb); @@ -1886,20 +1893,20 @@ tm_region_init (struct tm_region *region { bitmap_set_bit (visited_blocks, e->dest->index); VEC_safe_push (basic_block, heap, queue, e->dest); - gcc_assert (!e->dest->aux); /* FIXME: Remove me. */ /* If the current block started a new region, make sure that only the entry block of the new region is associated with this region. Other successors are still part of the old region. */ if (old_region != region && e->dest != region->entry_block) - e->dest->aux = old_region; + region_worklist[e->dest->index] = old_region; else - e->dest->aux = region; + region_worklist[e->dest->index] = region; } } while (!VEC_empty (basic_block, queue)); VEC_free (basic_block, heap, queue); BITMAP_FREE (visited_blocks); + free (region_worklist); } /* The "gate" function for all transactional memory expansion and optimization @@ -2424,6 +2431,42 @@ get_tm_region_blocks (basic_block entry_ return bbs; } +/* Set the IN_TRANSACTION for all gimple statements that appear in a + transaction. */ + +void +compute_transaction_bits (void) +{ + struct tm_region *region; + VEC (basic_block, heap) *queue; + unsigned int i; + gimple_stmt_iterator gsi; + basic_block bb; + + /* ?? Perhaps we need to abstract gate_tm_init further, because we + certainly don't need it to calculate CDI_DOMINATOR info. */ + gate_tm_init (); + + for (region = all_tm_regions; region; region = region->next) + { + queue = get_tm_region_blocks (region->entry_block, + region->exit_blocks, + region->irr_blocks, + NULL, + /*stop_at_irr_p=*/true); + for (i = 0; VEC_iterate (basic_block, queue, i, bb); ++i) + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) + { + gimple stmt = gsi_stmt (gsi); + gimple_set_in_transaction (stmt, true); + } + VEC_free (basic_block, heap, queue); + } + + if (all_tm_regions) + bitmap_obstack_release (&tm_obstack); +} + /* 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 Index: gimple.h =================================================================== --- gimple.h (revision 184271) +++ gimple.h (working copy) @@ -305,8 +305,10 @@ struct GTY(()) gimple_statement_base { /* Nonzero if this statement contains volatile operands. */ unsigned has_volatile_ops : 1; - /* Padding to get subcode to 16 bit alignment. */ - unsigned pad : 1; + /* Nonzero if this statement appears inside a transaction. This bit + is calculated on de-mand and has relevant information only after + it has been calculated with compute_transaction_bits. */ + unsigned in_transaction : 1; /* The SUBCODE field can be used for tuple-specific flags for tuples that do not require subcodes. Note that SUBCODE should be at @@ -1122,6 +1124,7 @@ extern tree omp_reduction_init (tree, tr /* In trans-mem.c. */ extern void diagnose_tm_safe_errors (tree); +extern void compute_transaction_bits (void); /* In tree-nested.c. */ extern void lower_nested_functions (tree); @@ -1586,6 +1589,21 @@ gimple_set_has_volatile_ops (gimple stmt stmt->gsbase.has_volatile_ops = (unsigned) volatilep; } +/* Return true if STMT is in a transaction. */ + +static inline bool +gimple_in_transaction (gimple stmt) +{ + return stmt->gsbase.in_transaction; +} + +/* Set the IN_TRANSACTION flag to TRANSACTIONP. */ + +static inline void +gimple_set_in_transaction (gimple stmt, bool transactionp) +{ + stmt->gsbase.in_transaction = (unsigned) transactionp; +} /* Return true if statement STMT may access memory. */