Re: [PATCH] Add option for dumping to stderr (issue6190057)

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

On Thu, Jun 14, 2012 at 8:58 AM, Sharad Singhai <singhai@google.com> wrote:
> Thanks for your comments. Responses inline.
>
> On Wed, Jun 13, 2012 at 4:48 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Fri, Jun 8, 2012 at 7:16 AM, Sharad Singhai <singhai@google.com> wrote:
>>> Okay, I have updated the attached patch so that the output from
>>> -ftree-vectorizer-verbose is considered diagnostic information and is
>>> always
>>> sent to stderr. Other functionality remains unchanged. Here is some
>>> more context about this patch.
>>>
>>> This patch improves the dump infrastructure and public interfaces so
>>> that the existing private pass-specific dump stream is separated from
>>> the diagnostic dump stream (typically stderr). ?The optimization
>>> passes can output information on the two streams independently.
>>>
>>> The newly defined interfaces are:
>>>
>>> Individual passes do not need to access the dump file directly. Thus Instead
>>> of doing
>>>
>>> ? if (dump_file && (flags & dump_flags))
>>> ? ? ?fprintf (dump_file, ...);
>>>
>>> they can do
>>>
>>> ? ? dump_printf (flags, ...);
>>>
>>> If the current pass has FLAGS enabled then the information gets
>>> printed into the dump file otherwise not.
>>>
>>> Similar to the dump_printf (), another function is defined, called
>>>
>>> ? ? ? ?diag_printf (dump_flags, ...)
>>>
>>> This prints information only onto the diagnostic stream, typically
>>> standard error. It is useful for separating pass-specific dump
>>> information from
>>> the diagnostic information.
>>>
>>> Currently, as a proof of concept, I have converted vectorizer passes
>>> to use the new dump format. For this, I have considered
>>> information printed in vect_dump file as diagnostic. Thus 'fprintf'
>>> calls are changed to 'diag_printf'. Some other information printed to
>>> dump_file is sent to the regular dump file via 'dump_printf ()'. It
>>> helps to separate the two streams because they might serve different
>>> purposes and might have different formatting requirements.
>>>
>>> For example, using the trunk compiler, the following invocation
>>>
>>> g++ -S v.cc -ftree-vectorize -fdump-tree-vect -ftree-vectorizer-verbose=2
>>>
>>> prints tree vectorizer dump into a file named 'v.cc.113t.vect'.
>>> However, the verbose diagnostic output is silently
>>> ignored. This is not desirable as the two types of dump should not interfere.
>>>
>>> After this patch, the vectorizer dump is available in 'v.cc.113t.vect'
>>> as before, but the verbose vectorizer diagnostic is additionally
>>> printed on stderr. Thus both types of dump information are output.
>>>
>>> An additional feature of this patch is that individual passes can
>>> print dump information into command-line named files instead of auto
>>> numbered filename. For example,
>>
>> I'd wish you'd leave out this part for a followup.
>
> I thought you wanted all parts together. Anyway, I can remove this part.
>
>>
>>>
>>> g++ -S -O2 v.cc -ftree-vectorize -fdump-tree-vect=foo.vect
>>> ? ? -ftree-vectorizer-verbose=2
>>> ? ? -fdump-tree-pre=foo.pre
>>>
>>> This prints the tree vectorizer dump into 'foo.vect', PRE dump into
>>> 'foo.pre', and the vectorizer verbose diagnostic dump onto stderr.
>>>
>>> Please take another look.
>>
>> --- tree-vect-loop-manip.c ? ? ?(revision 188325)
>> +++ tree-vect-loop-manip.c ? ? ?(working copy)
>> @@ -789,14 +789,11 @@ slpeel_make_loop_iterate_ntimes (struct loop *loop
>> ? gsi_remove (&loop_cond_gsi, true);
>>
>> ? loop_loc = find_loop_location (loop);
>> - ?if (dump_file && (dump_flags & TDF_DETAILS))
>> - ? ?{
>> - ? ? ?if (loop_loc != UNKNOWN_LOC)
>> - ? ? ? ?fprintf (dump_file, "\nloop at %s:%d: ",
>> + ?if (loop_loc != UNKNOWN_LOC)
>> + ? ?dump_printf (TDF_DETAILS, "\nloop at %s:%d: ",
>> ? ? ? ? ? ? ? ? ?LOC_FILE (loop_loc), LOC_LINE (loop_loc));
>> - ? ? ?print_gimple_stmt (dump_file, cond_stmt, 0, TDF_SLIM);
>> - ? ?}
>> -
>> + ?if (dump_flags & TDF_DETAILS)
>> + ? ?dump_gimple_stmt (TDF_SLIM, cond_stmt, 0);
>> ? loop->nb_iterations = niters;
>>
>> I'm confused by this. ?Why is this not simply
>>
>> ?if (loop_loc != UNKNOWN_LOC)
>> ? ?dump_printf (dump_flags, "\nloop at %s:%d: ",
>> ? ? ? ? ? ? ? ? ? ? ? LOC_FILE (loop_loc), LOC_LINE (loop_loc));
>> ?dump_gimple_stmt (dump_flags | TDF_SLIM, cond_stmt, 0);
>>
>> for example. ?I notice that you maybe mis-understood the message classification
>> I asked you to add (maybe I confused you by mentioning to eventually re-use
>> the TDF_* flags). ?I think you basically provided this message classification
>> by adding two classes by providing both dump_gimple_stmt and diag_gimple_stmt.
>> But still in the above you keep a dump_flags test _and_ you pass in
>> (altered) dump_flags to the dump/diag_gimple_stmt routines. ?Let me quote them:
>>
>> +void
>> +dump_gimple_stmt (int flags, gimple gs, int spc)
>> +{
>> + ?if (dump_file)
>> + ? ?print_gimple_stmt (dump_file, gs, spc, flags);
>> +}
>>
>> +void
>> +diag_gimple_stmt (int flags, gimple gs, int spc)
>> +{
>> + ?if (alt_dump_file)
>> + ? ?print_gimple_stmt (alt_dump_file, gs, spc, flags);
>> +}
>>
>> I'd say it should have been a single function:
>>
>> void
>> dump_gimple_stmt (enum msg_classification, int additional_flags,
>> gimple gs, int spc)
>> {
>> ?if (msg_classification & go-to-dumpfile
>> ? ? ?&& dump_file)
>> ? ?print_gimple_stmt (dump_file, gs, spc, dump_flags | additional_flags);
>> ?if (msg_classification & go-to-alt-dump-file
>> ? ? ?&& alt_dump_file && (alt_dump_flags & msg_classification))
>> ? ?print_gimple_stmt (alt_dump_file, gs, spc, alt_dump_flags |
>> additional_flags);
>> }
>
> Okay.
>
>> where msg_classification would include things to suitably classify messages
>> for -fopt-info, like MSG_MISSED_OPTIMIZATION, MSG_OPTIMIZED, MSG_NOTE.
>>
>> In another place we have
>>
>> @@ -1648,7 +1642,7 @@ vect_can_advance_ivs_p (loop_vec_info loop_vinfo)
>> ? /* Analyze phi functions of the loop header. ?*/
>>
>> ? if (vect_print_dump_info (REPORT_DETAILS))
>> - ? ?fprintf (vect_dump, "vect_can_advance_ivs_p:");
>> + ? ?diag_printf (TDF_TREE, "vect_can_advance_ivs_p:");
>>
>> so why's that diag_printf and why TDF_TREE? ?I suppose you made all
>> dumps to vect_dump diag_* and all dumps to dump_file dump_*? ?I
>> think it should have been
>
> Okay.
>
>>
>> ? dump_printf (REPORT_DETAILS, "vect_can_advance_ivs_p:");
>>
>> thus dump_printf only gets the msg-classification and the printf args
>> (dump-flags
>> are only meaningful when passed down to pretty-printers). ?Thus
>>
>> @@ -1658,8 +1652,8 @@ vect_can_advance_ivs_p (loop_vec_info loop_vinfo)
>> ? ? ? phi = gsi_stmt (gsi);
>> ? ? ? if (vect_print_dump_info (REPORT_DETAILS))
>> ? ? ? ?{
>> - ? ? ? ? ?fprintf (vect_dump, "Analyze phi: ");
>> - ? ? ? ? ?print_gimple_stmt (vect_dump, phi, 0, TDF_SLIM);
>> + ? ? ? ? ?diag_printf (TDF_TREE, "Analyze phi: ");
>> + ? ? ? ? ?diag_gimple_stmt (TDF_SLIM, phi, 0);
>> ? ? ? ?}
>>
>> should be
>>
>> ?dump_printf (REPORT_DETAILS, "Analyze phi: ");
>> ?dump_gimple_stmt (REPORT_DETAILS, TDF_SLIM, phi, 0);
>>
>> and the if (vect_print_dump_info (REPORT_DETAILS)) should be what
>> the dump infrastructure provides and thus hidden. ?Eventually we should
>> have a dump_kind (msg-classification) so we can replace it with
>
> I considered that, however, the vect_print_dump_info () uses
> 'vect_location' in a way that seems pass-specific. I didn't want to
> make generic dump infrastructure aware of vectorizer-specific
> reporting. Also, while more refactoring is possible, I don't know how
> much people rely on the vectorizer pass dump output format. Anyway, I
> will try to work around these limitations.

Ah, that's a good point - the dump_* routines should get a location argument
then (possibly mimcking what we do for tree builders, have the main
function be dump_*_loc (location_t, ...) and have a #define for dump_* which
passes UNKNOWN_LOCATION to dump_*_loc which can then refrain from
outputting any location prefix).

I suppose nobody relies on the vectorizer pass dump output format but
testcases in our testsuite.

>>
>> ? if (dump_kind (REPORT_DETAILS))
>> ? ? {
>> ? ? ? dump_printf (REPORT_DETAILS, "Analyze phi: ");
>> ? ? ? dump_gimple_stmt (REPORT_DETAILS, TDF_SLIM, phi, 0);
>> ? ? }
>>
>> to reduce the runtime overhead when not diagnosing/dumping.
>>
>> @@ -2464,8 +2458,8 @@ vect_create_cond_for_alias_checks (loop_vec_info l
>> ? ? }
>>
>> ? if (vect_print_dump_info (REPORT_VECTORIZED_LOCATIONS))
>> - ? ?fprintf (vect_dump, "created %u versioning for alias checks.\n",
>> - ? ? ? ? ? ? VEC_length (ddr_p, may_alias_ddrs));
>> + ? ?diag_printf (TDF_TREE, "created %u versioning for alias checks.\n",
>> + ? ? ? ? ? ? ? ? VEC_length (ddr_p, may_alias_ddrs));
>> ?}
>>
>> so here we have a different msg-classification,
>> REPORT_VECTORIZED_LOCATIONS. ?As we eventually want a central
>> -fopt-report=... we do not want to leave this implementation detail to
>> individual passes but gather them in a central place.
>
> Okay. If I understand your comment right, you want all the different
> REPORT_xxx handled in one place. Currently, vectorizer has about 10 of
> such types in flag_types.h. All of these will become different
> dump_kind flags under the new scheme and other passes will be free to
> add more classification flags in future. Is that correct? If so, a
> concern is that many of these flags could be very pass specific. For
> example, since REPORT_VECTORIZED_LOCATION is not meaningful to other
> passes, does it even deserve to be a msg classification?

Well, I suppose the generic name would be REPORT_OPTIMIZED_LOCATIONS
and REPORT_UNOPTIMIZED_LOCATIONS and if you dump information from
the vectorizer these naturally are about vectorized locations.  Thus,
-fopt-report=optimized,all for alll passes information on
optimizations or -fopt-report=optimized,vect for only vectorizer
information?  How do other compilers
allow to specify detail / transformation kind in this context?

Thus yes, try to use "common" report kinds, but I can envision that eventually
a few very specific ones will pop in.

Thanks,
Richard.

> Thanks,
> Sharad
>
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Sharad
>>>
>>>
>>> On Thu, Jun 7, 2012 at 12:19 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Wed, Jun 6, 2012 at 10:58 PM, Sharad Singhai <singhai@google.com> wrote:
>>>>> Sorry about the delay. I have finally incorporated all the suggestions
>>>>> and reorganized the dump infrastructure a bit. The attached patch
>>>>> updates vectorizer passes so that instead of accessing global
>>>>> dump_file directly, these passes call dump_printf (FLAG, format, ...).
>>>>> The dump_printf can choose between two streams, one regular pass dump
>>>>> file, and another optional command line provided file. Currently, it
>>>>> doesn't discriminate and all the dump information goes to both the
>>>>> streams.
>>>>>
>>>>> Thus, for example,
>>>>>
>>>>> g++ -O2 v.cc -ftree-vectorize -fdump-tree-vect=foo.v -ftree-vectorizer-verbose=3
>>>>
>>>> But the default form of dump option (-fdump-tree-vect) no longer
>>>> interferes with -ftree-vectorize-verbose=xxx output right? (this is
>>>> one of the main requirements). One dumped to the primary stream (named
>>>> dump file) and the other to the stderr -- the default diagnostic (alt)
>>>> stream.
>>>>
>>>> David
>>>>
>>>>>
>>>>> will output the verbose vectorizer information in both *.vect file and
>>>>> foo.v file. However, as I have converted only vectorizer passes so
>>>>> far, there is additional information in *.vect file which is not
>>>>> present in foo.v file. Once other passes are converted to use this
>>>>> scheme, then these two dump files should have identical output.
>>>>>
>>>>> Also note that in this patch -fdump-xxx=yyy format does not override
>>>>> any auto named dump files as in my earlier patches. Instead the dump
>>>>> information is output to both places when a command line dump file
>>>>> option is provided.
>>>>>
>>>>> To summarize:
>>>>> - instead of using dump_begin () / dump_end (), the passes should use
>>>>> dump_start ()/dump_finish (). These new variants do not return the
>>>>> dump_file. However, they still set the global dump_file/dump_flags for
>>>>> the benefit of other passes during the transition.
>>>>> - instead of directly printing to the dump_file, as in
>>>>> if (dump_file)
>>>>> ?fprintf (dump_file, ...);
>>>>>
>>>>> The passes should do
>>>>>
>>>>> dump_printf (dump_flag, ...);
>>>>> This will output to dump file(s) only when dump_flag is enabled for a
>>>>> given pass.
>>>>>
>>>>> I have bootstrapped and tested it on x86_64. Does it look okay?
>>>>>
>>>>> Thanks,
>>>>> Sharad
>>>>>
>>>>>
>>>>> On Mon, May 14, 2012 at 12:26 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Sat, May 12, 2012 at 6:39 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>> On Sat, May 12, 2012 at 9:26 AM, Gabriel Dos Reis
>>>>>>> <gdr@integrable-solutions.net> wrote:
>>>>>>>> On Sat, May 12, 2012 at 11:05 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>
>>>>>>>>> The downside is that the dump file format will look different from the
>>>>>>>>> stderr output which is less than ideal.
>>>>>>>>
>>>>>>>> BTW, why do people want to use stderr for dumping internal IRs,
>>>>>>>> as opposed to stdout or other files? ?That does not sound right.
>>>>>>>>
>>>>>>>
>>>>>>> I was talking about the transformation information difference. In
>>>>>>> stderr (where diagnostics go to), we may have
>>>>>>>
>>>>>>> foo.c: in function 'foo':
>>>>>>> foo.c:5:6: note: loop was vectorized
>>>>>>>
>>>>>>> but in dump file the format for the information may be different,
>>>>>>> unless we want to duplicate the machinery in diagnostics.
>>>>>>
>>>>>> So? ?What's the problem with that ("different" diagnostics)?
>>>>>>
>>>>>> Richard.
>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>> -- Gaby