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

Fri Aug 7 14:52:17 PDT 2020

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85493