From patchwork Tue Dec 13 15:37:30 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: PR/51443: fix inline assembly in transaction_callable functions Date: Tue, 13 Dec 2011 05:37:30 -0000 From: Aldy Hernandez X-Patchwork-Id: 131118 Message-Id: <4EE7713A.5070104@redhat.com> To: Richard Henderson , gcc-patches A while back we redesigned how the may go irrevocable bit was set. In particular, we redesigned it so that the analysis was done after IPA-inline was done, so we didn't make incorrect assumptions about irrevocability. This means that any set or use of the tm_may_enter_irr bit before we calculate it in the IPA-tm pass is incorrect. The following patch removes such references. As is customary, this unearthed a few other buglets and cleanups, which I have also addressed. First, saw_unsafe is no longer needed. I removed it. Second, ipa_tm_scan_irr_function is now called with builtin operators such as new, so we need to exit gracefully when no cfun or CFG exists. Lastly, since we are being slightly more aggressive in determining irrevocability, we need to make sure that user annotated safe functions are respected. This fixes the PR. OK? PR/51443 * trans-mem.c (struct diagnose_tm): Remove saw_unsafe. (diagnose_tm_1): Same. (ipa_tm_execute): Do not test tm_may_enter_irr before we set it. (ipa_tm_scan_irr_function): Return gracefully when no DECL_STRUCT_FUNCTION. (ipa_tm_scan_irr_block): Believe the user on TM attributes. Index: testsuite/gcc.dg/tm/asm-1.c =================================================================== --- testsuite/gcc.dg/tm/asm-1.c (revision 0) +++ testsuite/gcc.dg/tm/asm-1.c (revision 0) @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-fgnu-tm -O1" } */ + +static inline void asmfunc() +{ +__asm__(""); +} + +__attribute__((transaction_callable)) +void push() +{ + asmfunc(); +} Index: testsuite/g++.dg/tm/asm-1.c =================================================================== --- testsuite/g++.dg/tm/asm-1.c (revision 0) +++ testsuite/g++.dg/tm/asm-1.c (revision 0) @@ -0,0 +1,22 @@ +// { dg-do compile } +// { dg-options "-fgnu-tm -O1" } + +template class shared_ptr { +public: + shared_ptr() { + __asm__ (""); + } +}; +template class deque { +public: + void push_back() { + ::new _Tp(); + } +}; +class Bar { + __attribute__((transaction_callable)) void push(); + deque > events; +}; +void Bar::push() { + events.push_back(); +} Index: trans-mem.c =================================================================== --- trans-mem.c (revision 182244) +++ trans-mem.c (working copy) @@ -544,7 +544,6 @@ struct diagnose_tm unsigned int summary_flags : 8; unsigned int block_flags : 8; unsigned int func_flags : 8; - unsigned int saw_unsafe : 1; unsigned int saw_volatile : 1; gimple stmt; }; @@ -695,8 +694,6 @@ diagnose_tm_1 (gimple_stmt_iterator *gsi else if (d->func_flags & DIAG_TM_SAFE) error_at (gimple_location (stmt), "asm not allowed in % function"); - else - d->saw_unsafe = true; break; case GIMPLE_TRANSACTION: @@ -711,8 +708,6 @@ diagnose_tm_1 (gimple_stmt_iterator *gsi else if (d->func_flags & DIAG_TM_SAFE) error_at (gimple_location (stmt), "relaxed transaction in % function"); - else - d->saw_unsafe = true; inner_flags = DIAG_TM_RELAXED; } else if (gimple_transaction_subcode (stmt) & GTMA_IS_OUTER) @@ -727,8 +722,6 @@ diagnose_tm_1 (gimple_stmt_iterator *gsi else if (d->func_flags & DIAG_TM_SAFE) error_at (gimple_location (stmt), "outer transaction in % function"); - else - d->saw_unsafe = true; inner_flags |= DIAG_TM_OUTER; } @@ -748,8 +741,6 @@ diagnose_tm_1 (gimple_stmt_iterator *gsi walk_gimple_seq (gimple_transaction_body (stmt), diagnose_tm_1, diagnose_tm_1_op, &wi_inner); - - d->saw_unsafe |= d_inner.saw_unsafe; } } break; @@ -780,11 +771,6 @@ diagnose_tm_blocks (void) walk_gimple_seq (gimple_body (current_function_decl), diagnose_tm_1, diagnose_tm_1_op, &wi); - /* If we saw something other than a call that makes this function - unsafe, remember it so that the IPA pass only needs to scan calls. */ - if (d.saw_unsafe && !is_tm_safe_or_pure (current_function_decl)) - cgraph_local_info (current_function_decl)->tm_may_enter_irr = 1; - return 0; } @@ -3696,7 +3682,11 @@ ipa_tm_scan_irr_block (basic_block bb) break; d = get_cg_data (cgraph_get_node (fn)); - if (d->is_irrevocable) + + /* Return true if irrevocable, but above all, believe + the user. */ + if (d->is_irrevocable + && !is_tm_safe_or_pure (fn)) return true; } break; @@ -3880,6 +3870,11 @@ ipa_tm_scan_irr_function (struct cgraph_ VEC (basic_block, heap) *queue; bool ret = false; + /* Builtin operators (operator new, and such). */ + if (DECL_STRUCT_FUNCTION (node->decl) == NULL + || DECL_STRUCT_FUNCTION (node->decl)->cfg == NULL) + return false; + current_function_decl = node->decl; push_cfun (DECL_STRUCT_FUNCTION (node->decl)); calculate_dominance_info (CDI_DOMINATORS); @@ -4689,14 +4684,11 @@ ipa_tm_execute (void) /* Scan for calls that are in each transaction. */ ipa_tm_scan_calls_transaction (d, &tm_callees); - /* If we saw something that will make us go irrevocable, put it - in the worklist so we can scan the function later - (ipa_tm_scan_irr_function) and mark the irrevocable blocks. */ - if (node->local.tm_may_enter_irr) - { - maybe_push_queue (node, &irr_worklist, &d->in_worklist); - d->want_irr_scan_normal = true; - } + /* Put it in the worklist so we can scan the function + later (ipa_tm_scan_irr_function) and mark the + irrevocable blocks. */ + maybe_push_queue (node, &irr_worklist, &d->in_worklist); + d->want_irr_scan_normal = true; } pop_cfun (); @@ -4712,11 +4704,10 @@ ipa_tm_execute (void) a = cgraph_function_body_availability (node); d = get_cg_data (node); - /* If we saw something that will make us go irrevocable, put it - in the worklist so we can scan the function later - (ipa_tm_scan_irr_function) and mark the irrevocable blocks. */ - if (node->local.tm_may_enter_irr) - maybe_push_queue (node, &irr_worklist, &d->in_worklist); + /* Put it in the worklist so we can scan the function later + (ipa_tm_scan_irr_function) and mark the irrevocable + blocks. */ + maybe_push_queue (node, &irr_worklist, &d->in_worklist); /* Some callees cannot be arbitrarily cloned. These will always be irrevocable. Mark these now, so that we need not scan them. */