Message ID | CAFiYyc37y7Q_z_7vHdZe98s6kKpgTJi4_ehmk=6gVnirV0TFDA@mail.gmail.com |
---|---|
State | New |
Headers | show |
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. > > 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? 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
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
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. > >> >> 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 <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
On Fri, Jun 15, 2012 at 7:47 AM, Sharad Singhai <singhai@google.com> wrote: > 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. >> >>> >>> 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 <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
Apologies for the spam. Attempting to resend the patch after shrinking it. I have updated the attached patch to use a new dump message classification system for the vectorizer. It currently uses four classes, viz, MSG_OPTIMIZED_LOCATIONS, MSG_UNOPTIMIZED_LOCATION, MSG_MISSING_OPTIMIZATION, and MSG_NOTE. I have gone through the vectorizer passes and have converted each call to fprintf (dump_file, ....) to a message classification matching in spirit. Most often, it is MSG_OPTIMIZED_LOCATIONS, but occasionally others as well. For example, the following if (vect_print_dump_info (REPORT_DETAILS)) { fprintf (vect_dump, "niters for prolog loop: "); print_generic_expr (vect_dump, iters, TDF_SLIM); } gets converted to if (dump_kind (MSG_OPTIMIZED_LOCATIONS)) { dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, vect_location, "niters for prolog loop: "); dump_generic_expr (MSG_OPTIMIZED_LOCATIONS, TDF_SLIM, iters); } The asymmetry between the first printf and the second is due to the fact that 'vect_print_dump_info (xxx)' prints the location as a "side-effect". To preserve the original intent somewhat, I have converted the first call within a dump sequence to a dump_printf_loc (xxx) which prints the location while the subsequence calls within the same conditional get converted to the corresponding plain variants. I considered removing the support for alternate dump file, but ended up preserving it instead since it is needed for setting the alternate dump file to stderr for the case when -fopt-info is given but no dump file is available. The following invocation g++ ... -ftree-vectorize -fopt-info=4 dumps *all* available information to stderr. Currently, the opt-info level is common to all passes, i.e., a pass can't specify if wants a different level of diagnostic info. This can be added as an enhancement with a suitable syntax for selecting passes. I haven't fixed up the documentation/tests but wanted to get some feedback about the current state of patch before doing that. Thanks, Sharad On Fri, Jun 15, 2012 at 1:18 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Fri, Jun 15, 2012 at 7:47 AM, Sharad Singhai <singhai@google.com> wrote: >> 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. >>> >>>> >>>> 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 <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
--- 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