From patchwork Wed Nov 17 16:17:08 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Bug, c++/46269, trans-mem] internal compiler error in expand_block_tm of trans-mem.c Date: Wed, 17 Nov 2010 06:17:08 -0000 From: Aldy Hernandez X-Patchwork-Id: 71581 Message-Id: <20101117161707.GA15094@redhat.com> To: Richard Henderson Cc: 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