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); } 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