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. 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