[NewPM] Print 'Skipping pass' as pass instrumentation

[aeubanks]

Summary

If OptNoneInstrumentation prints it instead, 'Skipping pass' will print for even required passes.

[ychen]

Looks good. Please add a test case.

[aeubanks]

Add test

[ychen]

llvm/lib/Passes/StandardInstrumentations.cpp

314

We probabaly don't need to check adaptor&pass manager here because they are never skipped.

317

It would be more clear to also print the IR name as registerBeforeNonSkippedPassCallback did (using unwrapAndPrint) .

llvm/test/Feature/optnone-opt.ll

95 ↗

(On Diff #283951)

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.

[aeubanks]

Print IR name

[aeubanks]

llvm/lib/Passes/StandardInstrumentations.cpp

314

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.

llvm/test/Feature/optnone-opt.ll

95 ↗

(On Diff #283951)

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.

[ychen]

llvm/lib/Passes/StandardInstrumentations.cpp

314

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.

llvm/test/Feature/optnone-opt.ll

95 ↗

(On Diff #283951)

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.

[aeubanks]

Check pass manager instead of verifier

[aeubanks]

llvm/test/Feature/optnone-opt.ll

95 ↗

(On Diff #283951)

Sounds good, done.

[ychen]

LGTM. Thanks!

llvm/include/llvm/Passes/StandardInstrumentations.h

private:
   /// Stack of Module description, enough to print the module after a given
   /// pass.
   SmallVector<PrintModuleDesc, 2> ModuleDescStack;
   bool StoreModuleDesc = false;
 };
 
 class OptNoneInstrumentation {
 public:
-  OptNoneInstrumentation(bool DebugLogging) : DebugLogging(DebugLogging) {}
+  OptNoneInstrumentation() {}
   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 
 private:
   bool skip(StringRef PassID, Any IR);
-  bool DebugLogging;
 };
 
 // Debug logging for transformation and analysis passes.
 class PrintPassInstrumentation {
 public:
   PrintPassInstrumentation(bool DebugLogging) : DebugLogging(DebugLogging) {}
   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 
 private:
   bool DebugLogging;
 };
 
 /// This class provides an interface to register all the standard pass
 /// instrumentations and manages their state (if any).
 class StandardInstrumentations {
   PrintIRInstrumentation PrintIR;
   PrintPassInstrumentation PrintPass;
   TimePassesHandler TimePasses;
   OptNoneInstrumentation OptNone;
 
 public:
-  StandardInstrumentations(bool DebugLogging)
-      : PrintPass(DebugLogging), OptNone(DebugLogging) {}
+  StandardInstrumentations(bool DebugLogging) : PrintPass(DebugLogging) {}
 
   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 
   TimePassesHandler &getTimePasses() { return TimePasses; }
 };
 } // namespace llvm
 
 #endif

llvm/lib/Passes/StandardInstrumentations.cpp

bool OptNoneInstrumentation::skip(StringRef PassID, Any IR) {
   if (!EnableOptnone)
     return true;
   const Function *F = nullptr;
   if (any_isa<const Function *>(IR)) {
     F = any_cast<const Function *>(IR);
   } else if (any_isa<const Loop *>(IR)) {
     F = any_cast<const Loop *>(IR)->getHeader()->getParent();
   }
-  if (F && F->hasOptNone()) {
-    if (DebugLogging)
-      dbgs() << "Skipping pass: " << PassID << " (optnone)\n";
-    return false;
-  }
-  return true;
+  return !(F && F->hasOptNone());
 }
 
 void PrintPassInstrumentation::registerCallbacks(
     PassInstrumentationCallbacks &PIC) {
   if (!DebugLogging)
     return;
 
   std::vector<StringRef> SpecialPasses = {"PassManager"};
   if (!DebugPMVerbose)
     SpecialPasses.emplace_back("PassAdaptor");
 
+  PIC.registerBeforeSkippedPassCallback(
+      [SpecialPasses](StringRef PassID, Any IR) {
+        if (isSpecialPass(PassID, SpecialPasses))
+          return;
+
+        dbgs() << "Skipping pass: " << PassID << "\n";
+      });
+
   PIC.registerBeforeNonSkippedPassCallback(
       [SpecialPasses](StringRef PassID, Any IR) {
         if (isSpecialPass(PassID, SpecialPasses))
           return;
 
         dbgs() << "Running pass: " << PassID << " on ";
         unwrapAndPrint(IR, "", false, true);
       });