On Wed, Sep 12, 2012 at 12:30 PM, Richard Guenther <richard.guent...@gmail.com> wrote: > On Wed, Sep 12, 2012 at 10:12 AM, Sharad Singhai <sing...@google.com> wrote: >> Thanks for your comments. Please see my responses inline. >> >> On Tue, Sep 11, 2012 at 1:16 PM, Xinliang David Li <davi...@google.com> >> wrote: >>> Can you resend your patch in text form (also need to resolve the >>> latest conflicts) so that it can be commented inline? >> >> I tried to include inline patch earlier but my message was bounced >> back from patches mailing list. I am trying it again. >> >>> Please also provide as summary a more up-to-date description of >>> 1) Command line option syntax and semantics >> >> I added some documentation in the patch. Here are the relevant bits >> from invoke.texi. >> >> `-fdump-tree-SWITCH-OPTIONS=FILENAME' >> Control the dumping at various stages of processing the >> intermediate language tree to a file. The file name is generated >> by appending a switch-specific suffix to the source file name, and >> the file is created in the same directory as the output file. In >> case of `=FILENAME' option, the dump is output on the given file >> instead of the auto named dump files. >> ... >> >> `=FILENAME' >> Instead of an auto named dump file, output into the given file >> name. The file names `stdout' and `stderr' are treated >> specially and are considered already open standard streams. >> For example, >> >> gcc -O2 -ftree-vectorize -fdump-tree-vect-details=foo.dump >> -fdump-tree-pre=stderr file.c >> >> outputs vectorizer dump into `foo.dump', while the PRE dump >> is output on to `stderr'. If two conflicting dump filenames >> are given for the same pass, then the latter option >> overrides the earlier one. >> >> `-fopt-info-PASS' >> `-fopt-info-PASS-OPTIONS' >> `-fopt-info-PASS-OPTIONS=FILENAME' >> Controls optimization dumps from various passes. If the `-OPTIONS' >> form is used, OPTIONS is a list of `-' separated options which >> controls the details of the dump. If OPTIONS is not specified, it >> defaults to `optimized'. If the FILENAME is not specified, it >> defaults to `stderr'. Note that the output FILENAME will be >> overwritten in case of multiple translation units. If a combined >> output from multiple the translation units is desired, `stderr' >> should be used instead. >> >> The PASS could be one of the tree or rtl passes. The following >> options are available > > I don't like that we have -PASS here. That makes it awfully similar > to -fdump-PASS-OPTIONS=FILENAME. Are we merely having > -fopt-info because OPTIONS are "different"? > >> `optimized' >> Print information when a particular optimization is >> successfully applied. It is up to the pass to decide which >> information is relevant. For example, the vectorizer pass >> prints the location of loop which got vectorized. >> >> `missed' >> Print information about missed optimizations. Individual >> passes control which information to include in the output. >> For example, >> >> gcc -O2 -ftree-vectorize -fopt-info-tree-vect-missed > > At least for -PASS better names should be available. And given the > lack of pass support for the new scheme (apart from the vectorizer) > we should enumerate the set of -PASS values we accept, currently > -vec only. IMHO it does not make sense to provide -fopt-info for > the myriad of passes we have - usually only high-level ones are interesting > to the user? > >> will print information about missed vectorization >> opportunities on to stderr. >> >> `note' >> Print verbose information about optimizations, such as certain >> transformations, more detailed information about decisions >> etc. >> >> `details' >> Print detailed information from a particular pass. This >> includes OPTIMIZED, MISSED, and NOTE. For example, >> >> gcc -O2 -ftree-vectorize >> -fopt-info-tree-vect-details=vect.details >> >> outputs detailed optimization report from the vectorization >> pass into `vect.details'. > > Can options be chained? like -fopt-info-vec-missed-note? > >> `-fopt-info-tree-all' >> `-fopt-info-tree-all-OPTIONS' >> `-fopt-info-tree-all-OPTIONS=FILENAME' >> This is similar to `-fopt-info' but instead of a single pass, it >> applies the dump options to all the tree passes. If the FILENAME >> is provided, the dump from all the passes is concatenated, >> otherwise the dump is output onto `stderr'. If OPTIONS is omitted, >> it defaults to `optimized'. >> >> gcc -O3 -fopt-info-tree-all-optimized-missed=tree.optdump >> >> This will output information about missed optimizations as well as >> optimized locations from all the tree passes into `tree.optdump'. > > That should be -fopt-info, thus omitting -PASS should default to enabling > all. > >>> 2) New dumping APIs and semantics >> >> New dumping API: >> >> dump_kind_p ==> predicate for whether a particular type of dump is enabled >> or not. Uses both dump_flags (TDF_*) and opt_info_flags (MSG_*). >> >> There are two variants, dump_xxx and dump_xxx_loc, the _loc variant is >> similar, >> except it also prints the source location. The source location is >> useful in certain >> type of dumps, specially for -fopt-info. >> >> Most of these are straightforward. >> >> dump_gimple_stmt >> dump_gimple_stmt_loc >> dump_generic_expr >> dump_generic_expr_loc >> dump_printf >> dump_printf_loc >> dump_basic_block >> dump_combine_total_stats >> >> dump_start/dump_finish: >> >> Before beginning of each pass, one needs to call dump_start () >> and at the end must call dump_finish (). I have already added the required >> calls in passes.c. >> >> Instead of using dump_file directly, use one of the dump_* functions >> declared in dumpfile.h. For example, dump_basic_block (...) >> should be used for printing a basic block instead of accessing a dump stream >> directly. >> >>> 3) Conversion changes >> >> I have converted vectorizer passes to use the new API. Here is an example >> >> - if (vect_print_dump_info (REPORT_DETAILS)) >> - fprintf (vect_dump, "=== vect_do_peeling_for_loop_bound ==="); >> + if (dump_kind_p (MSG_OPTIMIZED_LOCATIONS)) >> + dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, vect_location, >> + "=== vect_do_peeling_for_loop_bound ==="); >> >> Instead of calling fprintf on vect_dump directly, use >> dump_printf_loc. This dump is done only when the >> MSG_OPTIMIZED_LOCATIONS dump is enabled. Both -fopt-info dumps and >> regular pass dumps are handled uniformly by treating MSG_* flags as >> extensions of dump flags. >> >> Unfortunately, for vectorizer passes the change was not mechanical. I had to >> decide for each individual dump site what type of dump was appropriate there. >> Another added wrinkle was that the source location was printed as a >> side-effect. >> Thus I had to convert some calls to use dump_*_loc variant, but not others. > > That was expected I guess. > >>> Looking at the patch briefly, I am confused with the opt-info syntax. >>> I thought the following is desired: >>> >>> -fopt-info=pass-flags >>> >>> where pass is the pass name, and flags is one of [optimized, notes, >>> missed]. Both pass and flags can be omitted. >>> >>> Is it implemented this way in your patch? >> >> Close to it but not quite the same way. >> I considered this syntax and in fact implemented it. But later updated it to >> use -fopt-info-PASS-FLAGS=filename instead, since it was consistent with the >> -fdump-PASS-FLAGS=filename. Thus it is very easy to specify opt info >> flags for a specific >> pass. For example, >> >> -fdump-tree-pre=filename ==> dump PRE pass info >> -fopt-info-tree-pre=filename ==> dump PRE optimization info >> >> Hopefully this syntax is not confusing. > > See above ;) I think the -fdump- interface is somewhat confusing, so we > should > take the chance to design something better for -fopt-info which is targeted at > GCC users and not developers. > > Aside from the -fopt-info flags management the patch is ok. To go forward > and allow others to adjust passes besides the vectorizer -fopt-info syntax > can be massaged as a followup. > > Thus, ok if it bootstraps and regtests ok.
Re-sent, shortened for the list where it hits the message limit ... you might want to send the vectorizer pieces separately. Richard.
