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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks marked an inline comment as done.
aeubanks added inline comments.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:314
+      [SpecialPasses](StringRef PassID, Any IR) {
+        if (isSpecialPass(PassID, SpecialPasses))
+          return;
----------------
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.


================
Comment at: llvm/test/Feature/optnone-opt.ll:95
+
+; VerifierPass should not be skipped
+; NPM-VERIFY-NOT: Skipping pass
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85493