[New PM][PassInstrumentation] IR printing support for New Pass Manager
Needs ReviewPublic

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

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)Thu, Sep 6, 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)Fri, Sep 21, 4:23 AM