From patchwork Wed Nov 17 16:17:08 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 71581 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 7EEF7B7191 for ; Thu, 18 Nov 2010 03:17:28 +1100 (EST) Received: (qmail 7933 invoked by alias); 17 Nov 2010 16:17:23 -0000 Received: (qmail 7911 invoked by uid 22791); 17 Nov 2010 16:17:20 -0000 X-SWARE-Spam-Status: No, hits=-6.2 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; Wed, 17 Nov 2010 16:17:13 +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.13.8/8.13.8) with ESMTP id oAHGHCRV031613 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 17 Nov 2010 11:17:12 -0500 Received: from redhat.com (vpn-9-211.rdu.redhat.com [10.11.9.211]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id oAHGH8Ru021705 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 17 Nov 2010 11:17:10 -0500 Date: Wed, 17 Nov 2010 11:17:08 -0500 From: Aldy Hernandez To: Richard Henderson Cc: gcc-patches@gcc.gnu.org Subject: Re: [Bug c++/46269] [trans-mem] internal compiler error in expand_block_tm of trans-mem.c Message-ID: <20101117161707.GA15094@redhat.com> References: <20101110214350.GA15556@redhat.com> <4CDC13AA.8050905@redhat.com> <20101111200206.GA26801@redhat.com> <4CDC4E92.7020004@redhat.com> <20101112142409.GA8529@redhat.com> <4CDD83AE.1050605@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4CDD83AE.1050605@redhat.com> User-Agent: Mutt/1.5.20 (2009-08-17) 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 Fri, Nov 12, 2010 at 10:13:02AM -0800, Richard Henderson wrote: > On 11/12/2010 06:24 AM, Aldy Hernandez wrote: > > - if (gimple_code (g) == GIMPLE_CALL) > > + if (gimple_code (g) == GIMPLE_ASM && region->irr_blocks) > > + bitmap_set_bit (region->irr_blocks, bb->index); > > This is wrong. An ASM does not transition to irrevocable, I won't undignify myself with a response to the rest of this email. Suffice to say that I'm an idiot. I should've quit while I was ahead and left the implementation to the interested reader... > This is a bug elsewhere. E.g. > > > if (is_tm_irrevocable (node->decl) > > || (a >= AVAIL_OVERWRITABLE > > && !tree_versionable_function_p (node->decl))) > > ipa_tm_note_irrevocable (node, &worklist); > > if (is_tm_irrevocable (node->decl) > || a <= AVAIL_NOT_AVAILABLE > || !tree_versionable_function_p (node->decl)) > ipa_tm_note_irrevocable (node, &worklist); This was actually the root of most everything. No need to initialize the worklist, if we will actually fill it with correct info in the first place :). Of course, this opened up all sorts of grief. For instance, we can't just mark not available functions as irrevocable, because they may be explicitly marked as safe or pure. On the other hand, callers of (assumed) irrevocable functions should not force the irrevocability bits up the call chain, if the (assumed) irrevocable function was specifically marked as safe/pure. The latter problem can be seen in c-c++-common/tm/safe-3.c. Anyways, look at the patch now. Feel free to continue crucifying me if I got it wrong again. Tested on x86-64 Linux. * gimple-pretty-print.c (dump_gimple_call): Add clone attribute to dump. * trans-mem.c (tm_region_init_1): Proceed even if there are not exit_blocks. (gate_tm_init): Initialize irr_blocks. (execute_tm_mark): Stop at irrevocable blocks for clones too. (ipa_tm_note_irrevocable): Do not mark callers as irrevocable if they are marked safe or pure. (ipa_tm_scan_irr_function): Only dump irrevocable blocks if we have them. (ipa_tm_execute): Rework irrevocable marking logic. PR/46269 * trans-mem.c (expand_block_tm): Handle GIMPLE_ASM. Index: testsuite/g++.dg/tm/pr46269.C =================================================================== --- testsuite/g++.dg/tm/pr46269.C (revision 0) +++ testsuite/g++.dg/tm/pr46269.C (revision 0) @@ -0,0 +1,29 @@ +// { dg-do compile } +// { dg-options "-fgnu-tm" } + +static inline void atomic_exchange_and_add() +{ + __asm__ ("blah"); +} + +template class shared_ptr +{ +public: + shared_ptr( T * p ) + { + atomic_exchange_and_add(); + } +}; + +class BuildingCompletedEvent +{ + public: + __attribute__((transaction_callable)) void updateBuildingSite(void); + __attribute__((transaction_pure)) BuildingCompletedEvent(); +}; + +void BuildingCompletedEvent::updateBuildingSite(void) +{ + shared_ptr event(new BuildingCompletedEvent()); +} + Index: gimple-pretty-print.c =================================================================== --- gimple-pretty-print.c (revision 166496) +++ gimple-pretty-print.c (working copy) @@ -541,6 +541,8 @@ dump_gimple_call (pretty_printer *buffer /* Dump the arguments of _ITM_beginTransaction sanely. */ if (TREE_CODE (fn) == ADDR_EXPR) fn = TREE_OPERAND (fn, 0); + if (TREE_CODE (fn) == FUNCTION_DECL && DECL_IS_TM_CLONE (fn)) + pp_string (buffer, " [tm-clone]"); if (TREE_CODE (fn) == FUNCTION_DECL && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL && DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_START Index: trans-mem.c =================================================================== --- trans-mem.c (revision 166604) +++ trans-mem.c (working copy) @@ -1732,7 +1732,8 @@ tm_region_init_1 (struct tm_region *regi gimple_stmt_iterator gsi; gimple g; - if (!region || !region->exit_blocks) + if (!region + || (!region->irr_blocks && !region->exit_blocks)) return region; /* Check to see if this is the end of a region by seeing if it @@ -1746,8 +1747,9 @@ tm_region_init_1 (struct tm_region *regi tree fn = gimple_call_fndecl (g); if (fn && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL) { - if (DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT - || DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT_EH) + if ((DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT + || DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT_EH) + && region->exit_blocks) { bitmap_set_bit (region->exit_blocks, bb->index); region = region->outer; @@ -1831,6 +1833,10 @@ gate_tm_init (void) obstack_alloc (&tm_obstack.obstack, sizeof (struct tm_region)); memset (region, 0, sizeof (*region)); region->entry_block = single_succ (ENTRY_BLOCK_PTR); + /* For a clone, the entire function is the region. But even if + we don't need to record any exit blocks, we may need to + record irrevocable blocks. */ + region->irr_blocks = BITMAP_ALLOC (&tm_obstack); tm_region_init (region); } @@ -2285,6 +2291,7 @@ execute_tm_mark (void) for (region = all_tm_regions; region ; region = region->next) { tm_log_init (); + /* If we have a transaction... */ if (region->exit_blocks) { unsigned int subcode @@ -2307,9 +2314,17 @@ execute_tm_mark (void) VEC_free (basic_block, heap, queue); } else + /* ...otherwise, we're a clone and the entire function is the + region. */ { FOR_EACH_BB (bb) - expand_block_tm (region, bb); + { + /* Stop at irrevocable blocks. */ + if (region->irr_blocks + && bitmap_bit_p (region->irr_blocks, bb->index)) + break; + expand_block_tm (region, bb); + } } tm_log_emit (); } @@ -3452,6 +3467,11 @@ ipa_tm_note_irrevocable (struct cgraph_n /* Don't examine recursive calls. */ if (e->caller == node) continue; + /* Even if we think we can go irrevocable, believe the user + above all. */ + if (is_tm_safe (e->caller->decl) + || is_tm_pure (e->caller->decl)) + continue; if (gimple_call_in_transaction_p (e->call_stmt)) d->want_irr_scan_normal = true; maybe_push_queue (e->caller, worklist_p, &d->in_worklist); @@ -3714,21 +3734,21 @@ ipa_tm_scan_irr_function (struct cgraph_ d->irrevocable_blocks_clone = new_irr; else d->irrevocable_blocks_normal = new_irr; + + if (dump_file) + { + const char *dname; + bitmap_iterator bmi; + unsigned i; + + dname = lang_hooks.decl_printable_name (current_function_decl, 2); + EXECUTE_IF_SET_IN_BITMAP (new_irr, 0, i, bmi) + fprintf (dump_file, "%s: bb %d goes irrevocable\n", dname, i); + } } else BITMAP_FREE (new_irr); - if (dump_file) - { - const char *dname; - bitmap_iterator bmi; - unsigned i; - - dname = lang_hooks.decl_printable_name (current_function_decl, 2); - EXECUTE_IF_SET_IN_BITMAP (new_irr, 0, i, bmi) - fprintf (dump_file, "%s: bb %d goes irrevocable\n", dname, i); - } - VEC_free (basic_block, heap, queue); pop_cfun (); current_function_decl = NULL; @@ -4394,12 +4414,19 @@ ipa_tm_execute (void) /* Some callees cannot be arbitrarily cloned. These will always be irrevocable. Mark these now, so that we need not scan them. */ - if (is_tm_irrevocable (node->decl) - || (a >= AVAIL_OVERWRITABLE - && !tree_versionable_function_p (node->decl))) + if (is_tm_irrevocable (node->decl)) + ipa_tm_note_irrevocable (node, &worklist); + else if (a <= AVAIL_NOT_AVAILABLE + && !is_tm_safe (node->decl) + && !is_tm_pure (node->decl)) ipa_tm_note_irrevocable (node, &worklist); - else if (a >= AVAIL_OVERWRITABLE && !d->is_irrevocable) - ipa_tm_scan_calls_clone (node, &tm_callees); + else if (a >= AVAIL_OVERWRITABLE) + { + if (!tree_versionable_function_p (node->decl)) + ipa_tm_note_irrevocable (node, &worklist); + else if (!d->is_irrevocable) + ipa_tm_scan_calls_clone (node, &tm_callees); + } } /* Iterate scans until no more work to be done. Prefer not to use