| Message ID | 20101110214350.GA15556@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/10/2010 01:43 PM, Aldy Hernandez wrote: > case GIMPLE_ASM: > - gcc_unreachable (); > + { > + gimple g; > + g = gimple_build_call (built_in_decls[BUILT_IN_TM_IRREVOCABLE], 0); > + gimple_set_location (g, gimple_location (stmt)); > + gsi_insert_before (&gsi, g, GSI_SAME_STMT); > + transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE); > + break; The reason there's a gcc_unreachable there is that we're supposed to have already handled this case. See ipa_tm_scan_irr_block where we notice the asm, and ipa_tm_scan_irr_blocks where we mark the block as irrevocable. See ipa_tm_transform_calls_1 where we insert the irrevocable call at the head of blocks so marked. In pass execute_tm_mark, we're supposed to have stopped transforming the region when the block is irrevocable. Thus the /*stop_at_irr_p=*/true in the call to get_tm_region_blocks. Apparently, that isn't doing what it says, so at a guess that's where the real bug lies. Assuming, of course, that we really did insert the irrevocable call during the ipa pass as we intended. r~
On Thu, Nov 11, 2010 at 08:02:50AM -0800, Richard Henderson wrote: > On 11/10/2010 01:43 PM, Aldy Hernandez wrote: > > case GIMPLE_ASM: > > - gcc_unreachable (); > > + { > > + gimple g; > > + g = gimple_build_call (built_in_decls[BUILT_IN_TM_IRREVOCABLE], 0); > > + gimple_set_location (g, gimple_location (stmt)); > > + gsi_insert_before (&gsi, g, GSI_SAME_STMT); > > + transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE); > > + break; > > The reason there's a gcc_unreachable there is that we're supposed to > have already handled this case. > > See ipa_tm_scan_irr_block where we notice the asm, and ipa_tm_scan_irr_blocks > where we mark the block as irrevocable. See ipa_tm_transform_calls_1 where > we insert the irrevocable call at the head of blocks so marked. Actually, that had been my first approach, but ipa_tm_scan_irr_function (and consequently ipa_tm_scan_irr_block*) only gets called for items in the worklist. And the worklist is only populated for: /* 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))) ipa_tm_note_irrevocable (node, &worklist); Since updateBuildingSite() is not makred as irrevocable, is_tm_irrevoable() returns false, and we never do the ipa_tm_scan_irr_block() thing you expect. I thought about teaching is_tm_irrevocable() to look at the tm_may_enter_irr bit you set in the IPA diagnose pass, but when that runs, we haven't run the inliner so we don't see the asm statement. Aldy
On 11/11/2010 12:02 PM, Aldy Hernandez wrote: > Actually, that had been my first approach, but ipa_tm_scan_irr_function > (and consequently ipa_tm_scan_irr_block*) only gets called for items in > the worklist. And the worklist is only populated for: > > /* 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))) > ipa_tm_note_irrevocable (node, &worklist); ... > Since updateBuildingSite() is not makred as irrevocable, > is_tm_irrevoable() returns false, and we never do the > ipa_tm_scan_irr_block() thing you expect. Um, no. This is for marking entire functions irrevocable. When the entire function is irrevocable, of course we don't scan the blocks. When the entire function is NOT so marked, then we scan the blocks looking for paths within the function that need to transition to serial-irrevocable. r~
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,30 @@ +// { dg-do compile } +// { dg-options "-fgnu-tm" } + +static inline void atomic_exchange_and_add() +{ + __asm__ ("blah"); +} + +template<class T> 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<BuildingCompletedEvent> event(new BuildingCompletedEvent()); +} + Index: trans-mem.c =================================================================== --- trans-mem.c (revision 166496) +++ trans-mem.c (working copy) @@ -2196,7 +2196,14 @@ expand_block_tm (struct tm_region *regio break; case GIMPLE_ASM: - gcc_unreachable (); + { + gimple g; + g = gimple_build_call (built_in_decls[BUILT_IN_TM_IRREVOCABLE], 0); + gimple_set_location (g, gimple_location (stmt)); + gsi_insert_before (&gsi, g, GSI_SAME_STMT); + transaction_subcode_ior (region, GTMA_MAY_ENTER_IRREVOCABLE); + break; + } default: break;