This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Attachment:
msg_classification.1.diff.bz2
Description: BZip2 compressed data
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |