On Wed, Feb 15, 2012 at 3:30 PM, Aldy Hernandez <al...@redhat.com> wrote:
> On 02/15/12 03:48, Richard Guenther wrote:
>>
>> On Tue, Feb 14, 2012 at 6:47 PM, Richard Henderson<r...@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.
