[New PM][PassInstrumentation] IR printing support for New Pass Manager
ClosedPublic

Authored by fedor.sergeev on Aug 17 2018, 1:57 PM.

Details

Summary

Implementing -print-before-all/-print-after-all/-filter-print-func support
through PassInstrumentation callbacks.

  • PrintIR routines implement printing callbacks.
  • StandardInstrumentations class provides a central place to manage all the "standard" in-tree pass instrumentations. Currently it registers PrintIR callbacks.

Diff Detail

Repository
rL LLVM
fedor.sergeev created this revision.Aug 17 2018, 1:57 PM

switching from IRUnitWrapper to llvm::Any

removing Before/After-Analysis callbacks mistakenly added to this commit

rebased on top of the changes in D47858

rebased on top of base changes. Management of PassInstrumentation object changed.
Now it just staysi locally on the same level as PassBuilder.

fedor.sergeev edited the summary of this revision. (Show Details)Sep 6 2018, 9:06 AM
fedor.sergeev edited the summary of this revision. (Show Details)

rebased

This implementation is modeled unnecessarily close to the legacy behavior. opt should just manually add the printer instrumentation if a print option is given. In particular., this shouldn't use the ShouldPrint* functions. This way there's no need to involve the PassBuilder API, and you also don't pay for the instrumentation if you don't want to print.

This implementation is modeled unnecessarily close to the legacy behavior. opt should just manually add the printer instrumentation if a print option is given.
In particular., this shouldn't use the ShouldPrint* functions.

I want all the debugging options to work the same manner whether it is opt or any other tool/way to run the pipeline
(say, in our JIT compiler we do sometimes use -print options). And I want them to work the same way both in legacy and new pass manager.
That means the main controlling interface should better be joined (and thats where ShouldPrint come into play).

This way there's no need to involve the PassBuilder API,

PassBuilder API is there again for an easy setup in tools *like* opt.
Somehow I thought providing a central way to install all the "blessed" callbacks would be adequate, even if right now there is only a single tool that calls it.

and you also don't pay for the instrumentation if you don't want to print.

Callbacks are not installed if printing is not enabled.
And empty instrumentation iteration is a price that we will always pay for simplicity sake. As discussed with Chandler.

I want all the debugging options to work the same manner whether it is opt or any other tool/way to run the pipeline
(say, in our JIT compiler we do sometimes use -print options). And I want them to work the same way both in legacy and new pass manager.
That means the main controlling interface should better be joined (and thats where ShouldPrint come into play).

Note that I'm not saying we should abandon the -print command line options, just that the ShouldPrint functions are the entirely wrong place to access them. This indirection is simply not necessary anymore.

This way there's no need to involve the PassBuilder API,

PassBuilder API is there again for an easy setup in tools *like* opt.
Somehow I thought providing a central way to install all the "blessed" callbacks would be adequate, even if right now there is only a single tool that calls it.

I understand the motivation. Given my oversight below I don't have very strong feelings about this anymore. Since I suspect this will also house the timing instrumentation registration, that's a stronger argument for this API then.

Callbacks are not installed if printing is not enabled.

You're right, I overlooked that bit.

rebased, removed PassBuilder::registerDefaultInstrumentation.
Subsequent patches will add a different way to manage default instrumentations.

See my comment to D52329. Analysis isn't the right place for this implementation. This should be part of the Passes library, and even live directly in the StandardInstrumentations implementation.

moving PrintIR callbacks into StandardInstrumentations.

fedor.sergeev edited the summary of this revision. (Show Details)Sep 21 2018, 4:23 AM

LGTM, but I wonder why the PIC object is set up and used only in this change. Did we miss that in the change introducing it?

tools/opt/NewPMDriver.cpp
221 ↗(On Diff #166443)

Why is this part of this particular change?

fedor.sergeev added inline comments.Sep 24 2018, 8:07 AM
tools/opt/NewPMDriver.cpp
221 ↗(On Diff #166443)

Our PassInstrumentation wrapper is capable of working with Callbacks == nullptr, and in that case it will essentially be no-op.
This change intends doing something real within default invocations of opt, so it intiializes PassBuilder with nontrivially initialized callbacks and that causes proper PassInstrumentationAnalyses being installed as part of the PassBuilder scrutiny.

fedor.sergeev marked an inline comment as done.Sep 24 2018, 8:08 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 24 2018, 9:10 AM
This revision was automatically updated to reflect the committed changes.