[PATCH] D47858: [New PM] Introducing PassInstrumentation framework

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: include/llvm/IR/PassInstrumentation.h:50
+/// (e.g. become unskippable). PassManager is the only entity that determines
+/// how pass instrumentation affects pass execution.
+///
----------------
fedor.sergeev wrote:
> andrew.w.kaylor wrote:
> > andrew.w.kaylor wrote:
> > > fedor.sergeev wrote:
> > > > philip.pfaffe wrote:
> > > > > andrew.w.kaylor wrote:
> > > > > > This is actually fairly important. There are many passes that will cause compilation to fail if they are skipped.
> > > > > > 
> > > > > > Maybe instead of having the pass managers not run the pass, you could add an argument to the run function that indicates the pass should be skipped if possible? That feels fairly clunky. The alternative is to have some way for the pass managers to detect that the passes cannot be skipped, and I suspect we don't want to do that either.
> > > > > Passes shouldn't be able to control this I think. That burden should be on the instrumentation.
> > > > This is marked as TODO :)
> > > > There are ways to allow passes opt-in/out of pass skipping, its just not a focus here.
> > > > After current series of patches will be landed I will start approaching OptBisect and then we will beat this dead horse till it is fully dead.
> > > >  
> > > But the instrumentation can't make this decision without having explicit knowledge about the pass. The pass itself is really the only thing that knows whether or not it can be skipped.
> > That's fair. It doesn't need to be solved now, but I think that until it is solved the default behavior should be to not skip any passes.
> There are still uses to the skip functionality even if it does not result in successful compilation.
> Say, we downstream have a patch that gracefully stops compilation when it exceeds some resource limit,
> and there we do not actually care about a successful compilation but just in graceful abort that does not leaks resources.
> 
> In this patch ability to skip the pass is more or less just a proof of concept.
> Instrumentations suggested in dependent patches (-print-after-all and -time-passes) do not use skipping ability either.
> 
> I would say - if it raises questions then I'm glad it is written this way :)
OK. I can live with it like this for now.

By the way, since I finally got around to commenting, I should say that I really like this design. Thanks for the work!


Repository:
  rL LLVM

https://reviews.llvm.org/D47858