[PATCH] D85493: [NewPM] Print 'Skipping pass' as pass instrumentation

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:314
+      [SpecialPasses](StringRef PassID, Any IR) {
+        if (isSpecialPass(PassID, SpecialPasses))
+          return;
----------------
aeubanks wrote:
> ychen wrote:
> > We probabaly don't need to check adaptor&pass manager here because they are never skipped.
> I could imagine them being skipped in the future. For example, an adaptor could check if the pass it's wrapping is required or not. So I think keeping this is fine.
If it is for the future and you're worried that it may not work as expected. I would suggest adding an assert to check the pass could not be special pass yet. Doing so we don't need to add a test for this and worry about the use cases for it.


================
Comment at: llvm/test/Feature/optnone-opt.ll:95
+
+; VerifierPass should not be skipped
+; NPM-VERIFY-NOT: Skipping pass
----------------
aeubanks wrote:
> ychen wrote:
> > Yeah, we don't have a required user pass yet to test. And I would rather not special-casing Verifer pass for this. How about, instead of doing this, extending `NPM-LOOP` check with IR names so we know the new code path is covered. We could defer the `required` passes testing for the future. There are some candidate machine passes that could be marked required.
> The point of this patch was that "Skipping pass" was printed when it shouldn't have been. Adding checks to `NPM-LOOP` doesn't really test that, `CHECK-NOT: Skipping pass` is really the important thing, and for that we need some required pass.
> I don't see what's wrong with making VerifierPass required in this patch as opposed to a patch that comes right after this.
> I noticed this as I was writing a test for making a bunch of passes required in preparation for turning on `-enable-npm-optnone` by default.
Then let's check pass managers or adaptors as the required pass (checking there are no `Skipping pass` for them or using CHECK-NEXT). In terms of code path, they make no difference from the required user pass. Making Verifier pass required could be a separate patch because we need to discuss the use cases for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85493