On Fri, Jun 15, 2012 at 7:47 AM, Sharad Singhai <sing...@google.com> wrote: > On Wed, Jun 13, 2012 at 4:48 AM, Richard Guenther > <richard.guent...@gmail.com> wrote: >> On Fri, Jun 8, 2012 at 7:16 AM, Sharad Singhai <sing...@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. >> >>> >>> 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); >> } > > Yes, I can make the single function work for both regular (dump_file) > and diagnostic (stderr for now) dump streams. In fact, my original > patch did just that. However, it duplicated output on *both* the > streams. For example, > > gcc -O2 -ftree-vectorize -fdump-tree-vect=foo.vect > -ftree-vectorizer-verbose=2 foo.c > > Since both streams are enabled in this case, a call to dump_printf () > would duplicate the information to both files, i.e., the dump and > diagnostic will be intermixed. Is that intended? My first thought was > to keep them separated via dump_*() and diag_* () methods.
No, I think that's intended. The regular dump-files should contain everything, the secondary stream (eventually enabled by -fopt-info) should be a filtered regular dump-file. Thanks, Richard. > Thanks, > Sharad > >> 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 >> >> 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 >> >> 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. >> >> Thanks, >> Richard. >> >>> Thanks, >>> Sharad >>> >>> >>> On Thu, Jun 7, 2012 at 12:19 AM, Xinliang David Li <davi...@google.com> >>> wrote: >>>> On Wed, Jun 6, 2012 at 10:58 PM, Sharad Singhai <sing...@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.guent...@gmail.com> wrote: >>>>>> On Sat, May 12, 2012 at 6:39 PM, Xinliang David Li <davi...@google.com> >>>>>> wrote: >>>>>>> On Sat, May 12, 2012 at 9:26 AM, Gabriel Dos Reis >>>>>>> <g...@integrable-solutions.net> wrote: >>>>>>>> On Sat, May 12, 2012 at 11:05 AM, Xinliang David Li >>>>>>>> <davi...@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