Page MenuHomePhabricator

Introduce new quiet mode and new option handling for -print-changed.
ClosedPublic

Authored by jamieschmeiser on Dec 3 2020, 10:40 AM.

Details

Summary

Introduce new quiet mode and new option handling for -print-changed.

Introduce a new mode of operation for -print-changed that only reports
after a pass changes the IR with all of the other messages suppressed (ie,
no initial IR and no messages about ignored, filtered or non-modifying
passes).

The option processing for -print-changed is changed to take an optional
string indicating options for print-changed. Initially, the only option
supported is quiet (as described above). This new quiet mode is specified
with -print-changed=quiet while -print-changed will continue to function
in the same way. It is intended that there will be more options in the
future.

Diff Detail

Event Timeline

jamieschmeiser created this revision.Dec 3 2020, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 10:40 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jamieschmeiser requested review of this revision.Dec 3 2020, 10:40 AM

Thanks, this is much more reviewable.

Should this be a flag toggle for print-changed? You'd never run -print-changed and -print-changed-only at the same time right? Perhaps defaulting to this for -print-changed, and adding a "verbose" option which would preserve the existing behavior would be nice. So it'd be something like the existing -debug-pass-manager-verbose flag. I vaguely remember using the -print-changed flag (I do like it a lot :) ) and being slightly annoyed with the verbosity of the ignored/special passes. WDYT?

I was thinking about a slightly different approach for these options. There are several different options (-print-changed, print-changed-diff, -dot-cfg-changed ) with different variations. As you say, it doesn't make sense to use more than one at a time so I was thinking that perhaps there could be -print-changed, and -print-changed-options=<options>. Without -print-changed-options, you would get this one (ie, before and after without banners for no changed, ignored, etc). The options would be a string of characters v being verbose (all banners), b is -print-before-changed, p is patch format (-print-changed-diff which could also have a verbose mode as currently up for review and default to skipping other banners). -dot-cfg-changed (also with/without verbose mode) would be d=string, b=string, a=string, c=string with strings going to the end of input or comma where d, b, a and c being the directory, before colour, after colour and common colour. The option letters can change, the important thing is the idea of the first option saying you want some form of change reporter (defaulting to this nonverbose print-changed version) and a modifier option that picks and controls the various types of reporter. WDYT?

I like that idea. Actually I think you might even be able to fit it all in one flag. Just passing -print-changed gives you the default, and you can say something like -print-changed=quiet to not print on skipped passes (as an example). And apparently this is possible with cl::opt, e.g. I found

static cl::opt<RunOutliner> EnableMachineOutliner(
    "enable-machine-outliner", cl::desc("Enable the machine outliner"),
    cl::Hidden, cl::ValueOptional, cl::init(TargetDefault),
    cl::values(clEnumValN(AlwaysOutline, "always",
                          "Run on all functions guaranteed to be beneficial"),
               clEnumValN(NeverOutline, "never", "Disable all outlining"),
               // Sentinel value for unspecified option.
               clEnumValN(AlwaysOutline, "", "")));
mamai added a subscriber: mamai.Dec 16 2020, 11:39 AM
jamieschmeiser retitled this revision from Introduce new option -print-changed-only and associated change printer. to Introduce new quiet mode and new option handling for -print-changed..
jamieschmeiser edited the summary of this revision. (Show Details)

I have changed the option handling as discussed and used the optional
string on the option as you suggested. I have also changed this from
being a separate change printer to being an option on the base class.
This way, they other proposed change printers can also benefit by getting
the verbose/quiet modes using the same code.

aeubanks accepted this revision.Dec 20 2020, 1:47 PM

LGTM!

This revision is now accepted and ready to land.Dec 20 2020, 1:47 PM