| Message ID | 4F3A8FF4.1070606@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/14/2012 08:46 AM, Aldy Hernandez wrote: > The call to ipa_tm_diagnose_tm_safe() does nothing because there are no longer any calls in the function, since the function call has been inlined: > > f () > { > <bb 2>: > __asm__ __volatile__(""); > return; > > } > > Perhaps we could issue the error when we notice the GIMPLE_ASM while scanning for irrevocable blocks earlier. The attached patch does so, and fixes the PR. > > What am I missing, cause I *know* there's a rat's nest somewhere. Ug. Which means that the error message is all too likely simply be confusing rather than anything else, since the asm isn't lexically present in the transaction. I wonder, not for the first time, if we shouldn't simply turn off early inlining with TM, or at least of and into tm-related functions, such as this. I assume that the IPA inlining pass would take up the slack... r~
On Tue, Feb 14, 2012 at 6:47 PM, Richard Henderson <rth@redhat.com> wrote: > On 02/14/2012 08:46 AM, Aldy Hernandez wrote: >> The call to ipa_tm_diagnose_tm_safe() does nothing because there are no longer any calls in the function, since the function call has been inlined: >> >> f () >> { >> <bb 2>: >> __asm__ __volatile__(""); >> return; >> >> } >> >> Perhaps we could issue the error when we notice the GIMPLE_ASM while scanning for irrevocable blocks earlier. The attached patch does so, and fixes the PR. >> >> What am I missing, cause I *know* there's a rat's nest somewhere. > > Ug. > > Which means that the error message is all too likely simply be confusing > rather than anything else, since the asm isn't lexically present in the > transaction. > > I wonder, not for the first time, if we shouldn't simply turn off early > inlining with TM, or at least of and into tm-related functions, such as > this. I assume that the IPA inlining pass would take up the slack... Hmm. I think you rather want to teach local_pure_const about TM properties you want to know and have them propagated properly (of course unless pure/const which is about optimization and has an easy fallback default yours wouldn't have that - you'd have to assume the callee contains an invalid asm ...) Richard. > > r~
On 02/15/12 03:48, Richard Guenther wrote: > On Tue, Feb 14, 2012 at 6:47 PM, Richard Henderson<rth@redhat.com> wrote: >> On 02/14/2012 08:46 AM, Aldy Hernandez wrote: >>> The call to ipa_tm_diagnose_tm_safe() does nothing because there are no longer any calls in the function, since the function call has been inlined: >>> >>> f () >>> { >>> <bb 2>: >>> __asm__ __volatile__(""); >>> return; >>> >>> } >>> >>> Perhaps we could issue the error when we notice the GIMPLE_ASM while scanning for irrevocable blocks earlier. The attached patch does so, and fixes the PR. >>> >>> What am I missing, cause I *know* there's a rat's nest somewhere. >> >> Ug. >> >> Which means that the error message is all too likely simply be confusing >> rather than anything else, since the asm isn't lexically present in the >> transaction. That's why I also specified the calling function in the error message. But I do agree that the message has the potential of confusing. >> >> I wonder, not for the first time, if we shouldn't simply turn off early >> inlining with TM, or at least of and into tm-related functions, such as >> this. I assume that the IPA inlining pass would take up the slack... You will hear no complaints from me. I'm tired of fixing the same bug over and over. > Hmm. I think you rather want to teach local_pure_const about TM > properties you want to know and have them propagated properly > (of course unless pure/const which is about optimization and has an > easy fallback default yours wouldn't have that - you'd have to assume > the callee contains an invalid asm ...) Richard G., can you explain the parenthesized comment. I'm not sure I follow you.
On Wed, Feb 15, 2012 at 3:30 PM, Aldy Hernandez <aldyh@redhat.com> wrote: > On 02/15/12 03:48, Richard Guenther wrote: >> >> On Tue, Feb 14, 2012 at 6:47 PM, Richard Henderson<rth@redhat.com> wrote: >>> >>> On 02/14/2012 08:46 AM, Aldy Hernandez wrote: >>>> >>>> The call to ipa_tm_diagnose_tm_safe() does nothing because there are no >>>> longer any calls in the function, since the function call has been inlined: >>>> >>>> f () >>>> { >>>> <bb 2>: >>>> __asm__ __volatile__(""); >>>> return; >>>> >>>> } >>>> >>>> Perhaps we could issue the error when we notice the GIMPLE_ASM while >>>> scanning for irrevocable blocks earlier. The attached patch does so, and >>>> fixes the PR. >>>> >>>> What am I missing, cause I *know* there's a rat's nest somewhere. >>> >>> >>> Ug. >>> >>> Which means that the error message is all too likely simply be confusing >>> rather than anything else, since the asm isn't lexically present in the >>> transaction. > > > That's why I also specified the calling function in the error message. But I > do agree that the message has the potential of confusing. > > >>> >>> I wonder, not for the first time, if we shouldn't simply turn off early >>> inlining with TM, or at least of and into tm-related functions, such as >>> this. I assume that the IPA inlining pass would take up the slack... > > > You will hear no complaints from me. I'm tired of fixing the same bug over > and over. > > >> Hmm. I think you rather want to teach local_pure_const about TM >> properties you want to know and have them propagated properly >> (of course unless pure/const which is about optimization and has an >> easy fallback default yours wouldn't have that - you'd have to assume >> the callee contains an invalid asm ...) > > > Richard G., can you explain the parenthesized comment. I'm not sure I > follow you. Ok, I had a look at the bugreport and am curios why __attribute__((always_inline)) static void asmfunc(void) { __asm__ (""); } __attribute__((transaction_safe)) static void f(void) { asmfunc(); } you'd want a diagnostic when asmfunc is not inlined but OTOH do not want to ICE when it is. Why not diagnose GIMPLE_ASMs alongside other 'unsafe function calls'. Thus, why does __attribute__((transaction_safe)) not transitively cover all its callees (does it?)? The comment of ipa_tm_diagnose_tm_safe says /* Diagnose calls from transaction_safe functions to unmarked functions that are determined to not be safe. */ and this "determined to not be safe" is what I was refering to with pointing to how we handle pure/const "discovery". For the asm above - how do you discover an asm is "unsafe"? I suppose you cannot, and you only consider asms lexically in a function marked as transaction-safe as safe? If so might I suggest to use a flag on the GIMPLE_ASM stmt, set during initial tm lowering (or even during parsing maybe)? Contrary to what rth says we _do_ have callgraph information available during early optimization / inlining. The callgraph is built over GENERIC already. So as you compute/propagate the tm_may_enter_irr flag only during IPA TM you might do better (with deciding what to allow to inline during early inlining, or other stuff) if you compute this function local property (conservatively) late in the early optimization pipeline, close to pass_local_pure_const (or simply integrated within it) - benefiting from trivial propagation ensured by the way we walk functions in the callgraph during early optimizations. The IPA TM pass then simply would do the propagation of the flag (no need to scan function bodies here, so this can even work at LTO level!) So, I guess the flag on the GIMPLE_ASM would be a way to fix 52141 (and probably the only sensible one?). The hint at the local-pure-const stuff would make IPA TM work at LTO level. Hope that helps ;) Richard.
[rth, please correct me if I have misrepresented the TM semantics and rules]. On 02/15/12 09:04, Richard Guenther wrote: > Ok, I had a look at the bugreport and am curios why > > __attribute__((always_inline)) > static void asmfunc(void) > { > __asm__ (""); > } > > __attribute__((transaction_safe)) > static void f(void) > { > asmfunc(); > } > > you'd want a diagnostic when asmfunc is not inlined but OTOH do not > want to ICE when it is. Why not diagnose GIMPLE_ASMs alongside > other 'unsafe function calls'. Thus, why does __attribute__((transaction_safe)) > not transitively cover all its callees (does it?)? No, transaction_safe does not transitively cover its callees. A transaction safe function cannot call unsafe functions, nor can GIMPLE_ASMs appear in it. (This was confusing for me at first too, because transaction_pure OTOH, does allow any type of call inside, or GIMPLE_ASM. For transaction_pure, no instrumentation is done, and the function is outputted as is.) > > The comment of ipa_tm_diagnose_tm_safe says > > /* Diagnose calls from transaction_safe functions to unmarked > functions that are determined to not be safe. */ > > and this "determined to not be safe" is what I was refering to with pointing > to how we handle pure/const "discovery". Interesting. I suppose we can add a cgraph bit for "this function may go irrevocable" and accumulate it during pure/const discovery. > For the asm above - how do you discover an asm is "unsafe"? I suppose > you cannot, and you only consider asms lexically in a function marked > as transaction-safe as safe? If so might I suggest to use a flag on the > GIMPLE_ASM stmt, set during initial tm lowering (or even during parsing maybe)? No, AFAIK, asms aren't safe even in transaction_safe functions (don't look at me, I didn't write the standard). But rth can correct me if I'm wrong. > So as you compute/propagate the tm_may_enter_irr flag only during > IPA TM you might do better (with deciding what to allow to inline during > early inlining, or other stuff) if you compute this function local property > (conservatively) late in the early optimization pipeline, close to > pass_local_pure_const (or simply integrated within it) - benefiting from > trivial propagation ensured by the way we walk functions in the callgraph > during early optimizations. The IPA TM pass then simply would do the > propagation of the flag (no need to scan function bodies here, so this can > even work at LTO level!) Nice. I like it. I will play around with it. > Hope that helps ;) Definitely, thanks! Aldy
On 02/15/2012 01:48 AM, Richard Guenther wrote: > Hmm. I think you rather want to teach local_pure_const about TM > properties you want to know and have them propagated properly > (of course unless pure/const which is about optimization and has an > easy fallback default yours wouldn't have that - you'd have to assume > the callee contains an invalid asm ...) That's more or less what we do ourselves during the tm ipa pass. The trouble we have here is merely one of diagnostics. The user has written "transaction safe" and it's now our job to tell him when he's done something that's not transaction safe. The unfortunate thing of diagnostics is you'd like to be able to tell the user what they did wrong at the source level, not after 10 levels of inlining. And given that pass_local_pure_const still runs after pass_early_inline, doing anything in there doesn't really help the problem at all. r~
On Wed, Feb 15, 2012 at 09:29:18AM -0800, Richard Henderson wrote: > On 02/15/2012 01:48 AM, Richard Guenther wrote: > > Hmm. I think you rather want to teach local_pure_const about TM > > properties you want to know and have them propagated properly > > (of course unless pure/const which is about optimization and has an > > easy fallback default yours wouldn't have that - you'd have to assume > > the callee contains an invalid asm ...) > > That's more or less what we do ourselves during the tm ipa pass. > > The trouble we have here is merely one of diagnostics. The user > has written "transaction safe" and it's now our job to tell him > when he's done something that's not transaction safe. > > The unfortunate thing of diagnostics is you'd like to be able to > tell the user what they did wrong at the source level, not after > 10 levels of inlining. > > And given that pass_local_pure_const still runs after pass_early_inline, > doing anything in there doesn't really help the problem at all. Use %K in error/warning fmt string, then the diagnostic will include the full virtual backtrace. Jakub
On 02/15/12 11:32, Jakub Jelinek wrote: > On Wed, Feb 15, 2012 at 09:29:18AM -0800, Richard Henderson wrote: >> On 02/15/2012 01:48 AM, Richard Guenther wrote: >>> Hmm. I think you rather want to teach local_pure_const about TM >>> properties you want to know and have them propagated properly >>> (of course unless pure/const which is about optimization and has an >>> easy fallback default yours wouldn't have that - you'd have to assume >>> the callee contains an invalid asm ...) >> >> That's more or less what we do ourselves during the tm ipa pass. >> >> The trouble we have here is merely one of diagnostics. The user >> has written "transaction safe" and it's now our job to tell him >> when he's done something that's not transaction safe. >> >> The unfortunate thing of diagnostics is you'd like to be able to >> tell the user what they did wrong at the source level, not after >> 10 levels of inlining. >> >> And given that pass_local_pure_const still runs after pass_early_inline, >> doing anything in there doesn't really help the problem at all. > > Use %K in error/warning fmt string, then the diagnostic will include the > full virtual backtrace. > > Jakub Hmmm, isn't %K for trees? We're talking gimple, and FUNCTION_DECLs, which don't have a TREE_BLOCK, here.
On Wed, Feb 15, 2012 at 11:59:15AM -0600, Aldy Hernandez wrote: > Hmmm, isn't %K for trees? We're talking gimple, and FUNCTION_DECLs, > which don't have a TREE_BLOCK, here. Gimple stmts have gimple_block, I guess you'd need to create some tree and set its TREE_BLOCK and EXPR_LOCATION from gimple_block/gimple_location. Or add something similar to %K that would take the same info from gimple stmt and pass a stmt instead of a tree. Jakub
Index: testsuite/gcc.dg/tm/pr52141.c =================================================================== --- testsuite/gcc.dg/tm/pr52141.c (revision 0) +++ testsuite/gcc.dg/tm/pr52141.c (revision 0) @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-options "-fgnu-tm -O1" } */ + +inline void asmfunc(void) +{ + __asm__ (""); /* { dg-error "asm not allowed in .transaction_safe" } */ +} + +__attribute__((transaction_safe)) +static void f(void) +{ + asmfunc(); +} + +int main() +{ + __transaction_atomic { + f(); + } + return 0; +} Index: trans-mem.c =================================================================== --- trans-mem.c (revision 184181) +++ trans-mem.c (working copy) @@ -3736,6 +3736,10 @@ ipa_tm_scan_irr_block (basic_block bb) assembly statement is not relevant to the transaction is to wrap it in a __tm_waiver block. This is not yet implemented, so we can't check for it. */ + if (is_tm_safe (current_function_decl)) + error_at (gimple_location (stmt), + "asm not allowed in %<transaction_safe%> function %qE", + current_function_decl); return true; default:
I swear, these inline asms in transactions are going to be the death of me. In the attached testcase diagnose_tm_blocks() and lower_tm() do not recognize the GIMPLE_ASM because early inlining has not yet run, so there is nothing to see. However, by the time we run the IPA TM pass, the assembly statement has been inlined but we no longer detect GIMPLE_ASMs correctly: /* Now validate all tm_safe functions, and all atomic regions in other functions. */ for (node = cgraph_nodes; node; node = node->next) if (node->reachable && node->lowered && cgraph_function_body_availability (node) >= AVAIL_OVERWRITABLE) { d = get_cg_data (&node, true); if (is_tm_safe (node->decl)) <-- !! FINDS NOTHING !! ipa_tm_diagnose_tm_safe (node); else if (d->all_tm_regions) ipa_tm_diagnose_transaction (node, d->all_tm_regions); } The call to ipa_tm_diagnose_tm_safe() does nothing because there are no longer any calls in the function, since the function call has been inlined: f () { <bb 2>: __asm__ __volatile__(""); return; } Perhaps we could issue the error when we notice the GIMPLE_ASM while scanning for irrevocable blocks earlier. The attached patch does so, and fixes the PR. What am I missing, cause I *know* there's a rat's nest somewhere. Aldy PR middle-end/52141 * trans-mem.c (ipa_tm_scan_irr_block): Error on GIMPLE_ASM.