Page MenuHomePhabricator

[NewPM] -print-before/-print-after support for new pass manager
Needs RevisionPublic

Authored by fedor.sergeev on Nov 21 2018, 6:01 AM.

Details

Summary

Existing print-before/after options are using PassNameParser which has only been able to parse legacy pass names,
rejecting everything else.

Majority of this fix is an enhancement for PassNameParser to
allow -print-before/after to take arbitrary strings, not just those
corresponding to legacy pass option names.

PassNameParser was converted to a template that takes boolean argument
'AllowArbitraryNames' that allows to just accept names that do not
match legacy passes.

As soon as these names are accepted by parser everything else works since
new-pm IR printing does already call llvm::shouldPrintBeforePass/shouldPrintAfterPass.

One important consequence of this implementation change is that there
is no more checking for errors in legacy pass names in -print-before/-print-after.
Other options that use PassNameParser do not set AllowArbitraryNames to true
and thus continue to check its values for errors.

Diff Detail

Event Timeline

fedor.sergeev created this revision.Nov 21 2018, 6:01 AM
fedor.sergeev edited the summary of this revision. (Show Details)Nov 21 2018, 6:39 AM

Is the goal here anything beyond makeing -print-after/before work in the newpm?

Because if it isn't, then this change is way too complicated, and the instrumentation should just directly implement shouldPrintAfterPass, etc.

Is the goal here anything beyond makeing -print-after/before work in the newpm?

Because if it isn't, then this change is way too complicated, and the instrumentation should just directly implement shouldPrintAfterPass, etc.

The goal is to use the same -print-after *option* to control this functionality in newpm, and not just reimplement it with a different control.

Eventually we should implement error checking for new-pm pass names.
That will require adding more new-pm related code to the parser, and introduce some
kind of an interface with PassBuilder. Definitely *not* planned for this patch.

I somewhat agree w/ Philip that this seems like too much complexity / machinery for what it does...

I'm personally not sure why we need to support what seems like a challenging interface to begin with to me...

I would personally be much more interested in making a single IR printing strategy that allows easy *post* processing along these lines. Here is a sketch of a design of something that would be more useful to me than these flags or the print-after-all flags, is sufficient to implement this logic with shell globs and cat, and likely not much more complex than this patch. Maybe even simpler:

Have a printer that accepts an optional directory parameter from the commandline. If unspecified, it creates a temporary director.

Within this directory, for each "nesting" pass run that is instrumented, it creates a directory named based on that pass name and a .N suffix where N is incremented from zero on each repeat of that pass at that level of nesting. While "inner" passes are running, set the target directory to the created one, and once finished, pop it back to the incoming target directory. For all passes, including "nesting" ones, write out a file <module-name>.<pass-name>.<run number at nest level>.{before,after}.ll that is a dump of the module.

Here, a "nesting" pass is just one that begins running, and another pass begins running before it ends running.

This seems easy to implement with some basic stack logic in the printer, etc.

If we want to get really fancy, give an option to write out a .tar or .zip file with this rather than an actual directory tree, and users can then implement these before/after things with a command to extract files matching a wildcard pattern.

What do you think?

In an IRC discussion, some interesting points were raised about this, and I wanted to try to give a better naming pattern and more concrete outline. I'll do it by giving a pseudo output tree I could imagine:

$PREFIX/mymodule.0.M2FA.before.ll
$PREFIX/mymodule.0.M2FA/0.foo.FPM.before.ll
$PREFIX/mymodule.0.M2FA/0.foo.FPM/0.InstCombine.before.ll
$PREFIX/mymodule.0.M2FA/0.foo.FPM/0.InstCombine.after.ll
$PREFIX/mymodule.0.M2FA/0.foo.FPM/1.SimplifyCFG.before.ll
$PREFIX/mymodule.0.M2FA/0.foo.FPM/1.SimplifyCFG.after.ll
$PREFIX/mymodule.0.M2FA/0.foo.FPM.after.ll
$PREFIX/mymodule.0.M2FA/1.bar.FPM.before.ll
$PREFIX/mymodule.0.M2FA/1.bar.FPM/0.InstCombine.before.ll
$PREFIX/mymodule.0.M2FA/1.bar.FPM/0.InstCombine.after.ll
$PREFIX/mymodule.0.M2FA/1.bar.FPM/1.SimplifyCFG.before.ll
$PREFIX/mymodule.0.M2FA/1.bar.FPM/1.SimplifyCFG.after.ll
$PREFIX/mymodule.0.M2FA/1.bar.FPM.after.ll
$PREFIX/mymodule.0.M2FA.after.ll

Here I'm using $PREFIX to mean "however we get a a directory". I guess either a flag value or createUniqueDirectory stuff.

I'm also using M2FA as a short name for the module-to-function pass adapter, and FPM for a function pass manager.

For the each .ll file, we'll also want to offer one specific level of filtering that can only really be implemented in the compiler: how much of the module goes into the file?

My suggestion would be that there are three reasonable options here:

  1. the entire module at that point in time
  2. the smallest enclosing IR unit to the IR unit being processed which we can do the equivalent of llvm-extract to produce a stand-alone module around. We could start with just extracting a function passes for function pass and smaller. But a follow-up would be to extract the set of functions in the SCC for SCC passes, etc. The key thing here is that we would do the extract and produce a valid module in all cases, just minimizing the IR in it.
  3. just the dump of the IR unit in question, despite it not necessarily being a complete module

While I think #3 is the right default for the print-after-all style flag, I think the default for the tree dump should be #2.

chandlerc requested changes to this revision.Dec 4 2018, 2:03 AM

(just marking that this is waiting on update from Fedor)

This revision now requires changes to proceed.Dec 4 2018, 2:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2020, 12:15 PM

This patch would be very useful (I stumbled upon it because I was about to do something similar) - any plans on reviving it?