[PATCH] D80692: Run Coverage pass before other *San passes under new pass manager, round 2

[Leonard Chan via Phabricator via cfe-commits]

leonardchan accepted this revision.
leonardchan added a comment.
This revision is now accepted and ready to land.

Would be worthwhile to write a small test that asserts Sancov runs before the other sanitizers under the new PM?

Aside from this, LGTM.



================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:597-598
   ///
   /// This extension point allows adding optimizations at the very end of the
   /// function optimization pipeline. A key difference between this and the
   /// legacy PassManager's OptimizerLast callback is that this extension point
----------------
aeubanks wrote:
> leonardchan wrote:
> > Will need to change the wording on this if this doesn't handle function passes anymore.
> This doesn't say that `registerOptimizerLastEPCallback()` handles function passes, it says it handles passes that will run right after all the default function optimization passes. I believe that's still true. The passes were previously added at the end of `OptimizePM`, now they're added right after `OptimizePM`, which is the same thing.
Ah I see. You're right. I misread.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1078
 
+  for (auto &C : OptimizerLastEPCallbacks)
+    C(MPM, Level);
----------------
aeubanks wrote:
> leonardchan wrote:
> > Would it be better to add another `SmallVector` and `register*Calback()` for modules in in `PassBuilder` that we could loop through here instead of changing how these extension points work?
> > 
> > I imagine it would still be meaningful in the future to be able to add function passes at the end of the function optimization pipeline.
> You can still add function passes via `createModuleToFunctionPassAdaptor()`, which is what I've done here for the existing usages. Given that the number of calls to `registerOptimizerLastEPCallback()` is small, I don't see a huge benefit to create a both a vector of module passes and function passes that run at the same place. If in the future we end up calling `registerOptimizerLastEPCallback()` with lots of function passes we can revisit this but for now it seems unnecessary.
> 
> This change doesn't change the ordering of any passes aside from sanitizer coverage (I believe), all usages of `registerOptimizerLastEPCallback()` will still end up putting the passes in the same place in the pipeline.
I see. Makes sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80692/new/

https://reviews.llvm.org/D80692