On Thu, Jun 28, 2018 at 4:29 PM David Malcolm <dmalc...@redhat.com> wrote: > > On Thu, 2018-06-28 at 13:29 +0200, Richard Biener wrote: > > On Tue, Jun 26, 2018 at 3:54 PM David Malcolm <dmalc...@redhat.com> > > wrote: > > > > > > On Mon, 2018-06-25 at 15:34 +0200, Richard Biener wrote: > > > > On Wed, Jun 20, 2018 at 6:34 PM David Malcolm <dmalc...@redhat.co > > > > m> > > > > wrote: > > > > > > > > > > Here's v3 of the patch (one big patch this time, rather than a > > > > > kit). > > > > > > > > > > Like the v2 patch kit, this patch reuses the existing dump API, > > > > > rather than inventing its own. > > > > > > > > > > Specifically, it uses the dump_* functions in dumpfile.h that > > > > > don't > > > > > take a FILE *, the ones that implicitly write to dump_file > > > > > and/or > > > > > alt_dump_file. I needed a name for them, so I've taken to > > > > > calling > > > > > them the "structured dump API" (better name ideas welcome). > > > > > > > > > > v3 eliminates v2's optinfo_guard class, instead using > > > > > "dump_*_loc" > > > > > calls as delimiters when consolidating "dump_*" calls. There's > > > > > a > > > > > new dump_context class which has responsibility for > > > > > consolidating > > > > > them into optimization records. > > > > > > > > > > The dump_*_loc calls now capture more than just a location_t: > > > > > they > > > > > capture the profile_count and the location in GCC's own sources > > > > > where > > > > > the dump is being emitted from. > > > > > > > > > > This works by introducing a new "dump_location_t" class as the > > > > > argument of those dump_*_loc calls. The dump_location_t can > > > > > be constructed from a gimple * or from an rtx_insn *, so that > > > > > rather than writing: > > > > > > > > > > dump_printf_loc (MSG_NOTE, gimple_location (stmt), > > > > > "some message: %i", 42); > > > > > > > > > > you can write: > > > > > > > > > > dump_printf_loc (MSG_NOTE, stmt, > > > > > "some message: %i", 42); > > > > > > > > > > and the dump_location_t constructor will grab the location_t > > > > > and > > > > > profile_count of stmt, and the location of the > > > > > "dump_printf_loc" > > > > > callsite (and gracefully handle "stmt" being NULL). > > > > > > > > > > Earlier versions of the patch captured the location of the > > > > > dump_*_loc call via preprocessor hacks, or didn't work > > > > > properly; > > > > > this version of the patch works more cleanly: internally, > > > > > dump_location_t is split into two new classes: > > > > > * dump_user_location_t: the location_t and profile_count > > > > > within > > > > > the *user's code*, and > > > > > * dump_impl_location_t: the __builtin_FILE/LINE/FUNCTION > > > > > within > > > > > the *implementation* code (i.e. GCC or a plugin), captured > > > > > "automagically" via default params > > > > > > > > > > These classes are sometimes used elsewhere in the code. For > > > > > example, "vect_location" becomes a dump_user_location_t > > > > > (location_t and profile_count), so that in e.g: > > > > > > > > > > vect_location = find_loop_location (loop); > > > > > > > > > > it's capturing the location_t and profile_count, and then when > > > > > it's used here: > > > > > > > > > > dump_printf_loc (MSG_NOTE, vect_location, "foo"); > > > > > > > > > > the dump_location_t is constructed from the vect_location > > > > > plus the dump_impl_location_t at that callsite. > > > > > > > > > > In contrast, loop-unroll.c's report_unroll's "locus" param > > > > > becomes a dump_location_t: we're interested in where it was > > > > > called from, not in the locations of the various dump_*_loc > > > > > calls > > > > > within it. > > > > > > > > > > Previous versions of the patch captured a gimple *, and needed > > > > > GTY markers; in this patch, the dump_user_location_t is now > > > > > just a > > > > > location_t and a profile_count. > > > > > > > > > > The v2 patch added an overload for dump_printf_loc so that you > > > > > could pass in either a location_t, or the new type; this > > > > > version > > > > > of the patch eliminates that: they all now take > > > > > dump_location_t. > > > > > > > > > > Doing so required adding support for rtx_insn *, so that one > > > > > can > > > > > write this kind of thing in RTL passes: > > > > > > > > > > dump_printf_loc (MSG_NOTE, insn, "foo"); > > > > > > > > > > One knock-on effect is that get_loop_location now returns a > > > > > dump_user_location_t rather than a location_t, so that it has > > > > > hotness information. > > > > > > > > > > Richi: would you like me to split out this location-handling > > > > > code into a separate patch? (It's kind of redundant without > > > > > adding the remarks and optimization records work, but if that's > > > > > easier I can do it) > > > > > > > > I think that would be easier because it doesn't require the JSON > > > > stuff and so I'll happily approve it. > > > > > > > > Thus - trying to review that bits (and sorry for the delay). > > > > > > > > + location_t srcloc = loc.get_location_t (); > > > > + > > > > if (dump_file && (dump_kind & pflags)) > > > > { > > > > - dump_loc (dump_kind, dump_file, loc); > > > > + dump_loc (dump_kind, dump_file, srcloc); > > > > print_gimple_stmt (dump_file, gs, spc, dump_flags | > > > > extra_dump_flags); > > > > } > > > > > > > > if (alt_dump_file && (dump_kind & alt_flags)) > > > > { > > > > - dump_loc (dump_kind, alt_dump_file, loc); > > > > + dump_loc (dump_kind, alt_dump_file, srcloc); > > > > print_gimple_stmt (alt_dump_file, gs, spc, dump_flags | > > > > extra_dump_flags); > > > > } > > > > + > > > > + if (optinfo_enabled_p ()) > > > > + { > > > > + optinfo &info = begin_next_optinfo (loc); > > > > + info.handle_dump_file_kind (dump_kind); > > > > + info.add_stmt (gs, extra_dump_flags); > > > > + } > > > > > > > > seeing this in multiple places. I seem to remember that > > > > dump_file / alt_dump_file was suposed to handle dumping > > > > into two locations - a dump file and optinfo (or stdout). This > > > > looks > > > > like the optinfo "stream" is even more separate. Could that > > > > obsolete the alt_dump_file stream? I'd need to review existing > > > > stuff > > > > in more detail to answer but maybe you already know from recently > > > > digging into this. > > > > > > Possibly. I attempted this in v1 of the patch, but it was mixed in > > > with > > > a bunch of other stuff. I'll have another go at doing this. > > > > > > > Oh, and all the if (optinfo_enable_p ()) stuff is for the > > > > followup > > > > then, right? > > > > > > Yes. > > > > > > > I like the boiler-plate changes to dump_* using stuff a lot, so > > > > the > > > > infrastructure to do that (the location wrapping) and these > > > > boiler- > > > > plate > > > > changes are pre-approved if split out. > > > > > > Thanks. I split out the location wrapping, and have committed it > > > to > > > trunk (r262149). It's not clear to me exactly what other parts you > > > liked, > > > so I'm going to try to split out more of the non-JSON bits in the > > > hope that some parts are good enough as-is, and I'll post them for > > > review > > > as followups. > > > > > > For reference, here's what I've committed (I added some obvious > > > changes > > > to doc/optinfo.texi). > > > > > > > I think the *_REMARK stuff should get attention of the respective > > > > maintainers - not sure what the difference between NOTE and > > > > REMARK > > > > is ;) > > > > > > Me neither :) I think it might come down to "this is purely a > > > debugging > > > message for a pass maintainer" (MSG_NOTE) vs "this is a high-level > > > thing > > > that an advanced user want to see" (MSG_REMARK???). > > > > > > One idea that occurred to me: are we overusing dump_flags_t? It's > > > a > > > mixture of TDF_* bitfields (used by our internal dumps) plus MSG_* > > > bitfields (used with -fopt-info). IIRC the only TDF_ bitfield > > > that's > > > ever used with the MSG_* bitfields is TDF_DETAILS. Might it make > > > sense > > > to split out the MSG_* bitfields into a different type > > > (dump_level_t???) > > > to reinforce this split? (and thus the dump_* entrypoints that > > > don't take > > > a FILE * would take this new type). I'm not sure if this is a good > > > idea > > > or not. > > > > Making it even more complicated doesn't make it easier to use it > > "correctly". So I'd rather try to simplify it. How passes use > > TDF_DETAILS vs. non-details is already highly inconsistent. This > > is why I liked the original optinfo work because it somehow made > > user-interesting vs. developer-interesting with the same API > > (and to the same dump-file). Just that MSG_NOTE is exposed to > > users while I think it should be developer-only... > > > > IMHO the TDF_ stuff should control things at the stmt/tree/BB level, > > thus be IL specific flags while the MSG_ stuff should control > > pass-specific dumping detail. > > You mention user-interesting vs developer-interesting, and whether it > would be the same API and/or the same dump-file. > > I've posted a bunch of patches here, some small, some large, trying > various different appoaches, coming at the problem from at least > two directions, so maybe it's worth discussing the overall direction > here (with some ASCII art!) > > * what is the status quo? > * what do we want to achieve? > * how do we get there? > > Status quo: > * We have the "dump_*(MSG_*)" API which hides the destination of > where we're dumping to (currently dump_file and/or alt_file_file, > conditionalized via flags). > > dump_* (MSG_*) ----> dumpfile.c --+--> dump_file > | > `--> alt_dump_file > > * As of r262149 (the dump_location_t commit) dumpfile.c receives the > hotness and emission location of each dump_*_loc call, but currently > it does nothing with that information. > * The messages that we emit through the "dump_*(MSG_*)" API and their > "levels" are currently rather haphazard. Some are close to being > suitable for end-users, but most seem never intended to be > end-user-facing. For reference: > grep -nH -e MSG_MISSED_OPTIMIZATION *.c *.cc | wc -l > 452 > grep -nH -e MSG_NOTE *.c *.cc | wc -l > 551 > grep -nH -e MSG_OPTIMIZED_LOCATIONS *.c *.cc | wc -l > 39 > (though some of these are support code e.g. in dumpfile.c, rather > than uses).
Yep. I believe MSG_NOTE was the fallout of optinfo introduction and my request to share the same API with dumping to the dumpfile. MSG_NOTE just received "anything else" ... > * The API builds up messages programatically, which is hostile to > i18n. The API isn't marked as needing i18n for its strings (which > IIRC is via using "gmsgid" as a param name being special-cased in > the gettext toolchain). > > What do we want to achieve? > * I want end-users to be able to enable high-level dump messages about > optimizations, and for those messages to be decipherable without > reading GCC sources e.g. the opt_problem idea posted here: > * "[PATCH] [RFC] Higher-level reporting of vectorization problems" > * https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01462.html > Presumably such messages would need to be internationalized. Many > other messages are really internal only, and would be a burden to > impose on our translators. > * I want to support additional destinations for any/all dump messages > beyond just the dump file and -fopt-info: > * putting them through the diagnostics subsystem (as "remarks") > * storing them to disk in a machine-readable format, so that > 3rd-party tools can present useful visualizations of how the > user's code is being optimized, e.g. prioritized by hotness > * I want to preserve the existing API and most existing uses of it > (to minimize churn and thus keep our lives easier) > > The patches I've been posting so far add additional dump destinations, > like this: > > dump_* (MSG_*) >---> dumpfile.c --+--> dump_file > | > +--> alt_dump_file > | > +--> diagnostic "remarks" > | > +--> "optimization records" > (saved to disk) > > I believe you favor an approach of "MSG_NOTE is internal, whereas > MSG_MISSED_OPTIMIZATION and MSG_OPTIMIZED_LOCATIONS are for end users". Yeah, kind of. You introduced another MSG_ variant and in the end we might need to go that way. I guess renaming MSG_NOTE to MSG_DEBUG would be a step in the direction to reflect reality ;) > I think the "should this dump message be internationalized?" issue > introduces a natural separation between dump messages aimed at > end-users vs purely "internal" dumps; this suggests to me that > we need a new API for such dump messages, designed to support i18n, > and marked as such for gettext, so I favor something like this: > > SOURCES DESTINATIONS > dump_* (MSG_*) >---> dumpfile.c --+--> dump_file > | | > new dump API -->-+ +--> alt_dump_file > | > +--> diagnostic "remarks" > | > +--> "optimization records" > (saved to disk) > > (I'm deliberately being vague about what this i18n-enabled dump API > might look like) I know I threw i18n into the discussion - but I'd say we should focus on other details first. I think there's no need to _require_ the "debug" dumpfile stuff not be translated, but to make it easier for translators I suppose being able to "amend" the API calls with a "do not translate" variant would be OK. So in what way is the dump_* API not suitable for translation that the diagnostic machinery (warning/error) does not suffer from? That is, if we internally make dump_* go through the pretty-printers we can handle stuff like quoting or even stmts/expressions. I think that would be useful for dumping to dumpfiles as well. > > That said, I have a version of the patch kit which does just this: > > dump_* (MSG_*) ----> dumpfile.c --+--> dump_file > | > +--> alt_dump_file > | > +--> diagnostic "remarks" > > i.e. generalizing the dump destinations (by adding optinfo internally), > but without requiring the JSON support or touching the dump API . Would > that be suitable as a next step? Yes. Thanks for going step-by-step btw, this is really useful in simplifying the picture. Richard. > > [...snip...] > > Thoughts? > > Thanks > Dave >