Re: New dump infrastructure

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Oct 17, 2012 at 6:55 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Wed, Oct 17, 2012 at 2:08 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Oct 17, 2012 at 9:05 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> A more simpler use model is not to guard the dump statement at all --
>>> just express the intention a) what to dump; b) as what kind or to
>>> where
>>>
>>>
>>> 1) I want to dump the something as optimized message:
>>>
>>>   dump_printf (MSG_OPTIMIZED, "blah...")
>>>
>>>   dump_printf_loc (MSG_OPTIMIZED, "blah")
>>>
>>> 2) I want to dump something together with tree dump regardless of the flag:
>>>
>>>    dump_printf (TDF_TREE, ...);
>>>
>>> 3) I want to dump something with tree dump when detailed flags is set:
>>>
>>>   dump_printf (TDF_DETAILS, ...);
>>>
>>>
>>> The dumping code is not in the critical path, so the compile time
>>> should not be a concern.
>>
>> That's not true in all cases which is why I asked for the predicate to be
>> available, especially for the case where it guards multiple dump_* statement.
>>
>> Look for example at CCPs ccp_visit_stmt which calls
>>
>>   if (dump_file && (dump_flags & TDF_DETAILS))
>>     {
>>       fprintf (dump_file, "\nVisiting statement:\n");
>>       print_gimple_stmt (dump_file, stmt, 0, dump_flags);
>>     }
>>
>
> Yes -- that means guarding is for performance reason and can be made optional.
>
>> and ends up calling evaluate_stmt most of the time:
>
> That looks too expensive even with the guard. Can it be turned off in
> non debug build?
>
>>
>>   if (dump_file && (dump_flags & TDF_DETAILS))
>>     {
>>       fprintf (dump_file, "which is likely ");
>>       switch (likelyvalue)
>>         {
>>         case CONSTANT:
>>           fprintf (dump_file, "CONSTANT");
>>           break;
>>         case UNDEFINED:
>>           fprintf (dump_file, "UNDEFINED");
>>           break;
>>         case VARYING:
>>           fprintf (dump_file, "VARYING");
>>           break;
>>         default:;
>>         }
>>       fprintf (dump_file, "\n");
>>     }
>>
>> such code is not something you'd want to do unconditionally.
>>
>
> ok.
>
>
>> I agree the use of the predicate can be reduced in some cases but it has
>> to be available for cases like the above.  And eventually have a fast-path
>> inlined like
>>
>> inline bool dump_kind_p (int flags)
>> {
>>   return any_dump_enabled_p ? dump_kind_p_1 (flags) : false;
>> }
>>
>> or a new inline function
>>
>> inline bool dump_enabled_p ()
>> {
>>   return any_dump_enabled_p;
>> }
>>
>> at the cost of one extra global flag we'd need to maintain.  Probably the
>> latter would be what you prefer?  That way we don't get redundant
>> or even conflicting flags on the predicate check vs. the dump stmts.
>>
>
> Another way:
>
> To make the dumping code as concise as possible, we can make
> dump_printf a inlinable wrapper:
>
> inline void dump_printf (...)
> {
>    if (!any_dump_enabled_p)
>       return;
>
>   // regular stuff
> }

You still have the issue that // regular stuff may appear to possibly
clobber any_dump_enabled_p and thus repeated any_dump_enabled_p
checks are not optimized by the compiler (we don't have predicated
value-numbering (yet)).

So I prefer the guard.  I suppose after this discussion I prefer
a any_dump_enabled_p () guard instead of one with (repeated) flags.

Richard.

> thanks,
>
> David
>
>
>> Thanks,
>> Richard.
>>
>>> David
>>>
>>>
>>> On Tue, Oct 16, 2012 at 11:42 PM, Sharad Singhai <singhai@google.com> wrote:
>>>>> 1. OK, I understand that e.g.
>>>>>
>>>>>      if (dump_file && (dump_flags & TDF_DETAILS))
>>>>>
>>>>>    should be converted into:
>>>>>
>>>>>      if (dump_kind_p (TDF_DETAILS))
>>>>>
>>>>>    But what about current code that does not care about dump_flags?
>>>>>    E.g. converting simple
>>>>>
>>>>>      if (dump_file)
>>>>>
>>>>>    to
>>>>>
>>>>>      if (dump_kind_p (0))
>>>>>
>>>>>    won't work, dump_kind_p will always return zero in such cases.
>>>>
>>>>
>>>> Yes, you are right, the conversion is not completely mechanical and
>>>> some care must be taken to preserve the original intent. I think one
>>>> of the following might work in the case where a pass doesn't care
>>>> about the dump_flags
>>>>
>>>> 1. use generic pass type flags, such as TDF_TREE, TDF_IPA, TDF_RTL
>>>> which are guaranteed to be set depending on the pass type,
>>>> 2. this dump statement might be a better fit for MSG_* flags if it
>>>> deals with optimizations. Sometimes "if (dump_file) fpritnf
>>>> (dump_file, ...)" idiom was used for these situations and now these
>>>> sites might be perfect candidate for converting into MSG_* flags.
>>>>
>>>> If a cleaner way to handle this is desired, perhaps we can add an
>>>> unconditional "dump_printf_always (...)", but currently it seems
>>>> unnecessary. Note that for optimization messages which should always
>>>> be printed, one can use MSG_ALL flag. However, no analogous flag
>>>> exists for regular dumps. How about adding a corresponding TDF_ALL
>>>> flag? Would that work?
>>>>
>>>>>
>>>>>
>>>>> 2. dump_kind_p seems to always return 0 if current_function_decl is
>>>>>    NULL.  However, that precludes its use in IPA passes in which this
>>>>>    can happen regularly.  Why is this restriction necessary?
>>>>
>>>>
>>>> This is an oversight on my part. Originally, I wanted to be able to
>>>> print source location information and this is a remnant of that. I am
>>>> testing a patch to fix that.
>>>>
>>>> Thanks,
>>>> Sharad