[PATCH] D87216: [NewPM] Support --print-before/after in NPM

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:449
+  // We currently only use these for --print-before/after.
+  if (PIC && (!printBeforePasses().empty() || !printAfterPasses().empty())) {
+#define MODULE_PASS(NAME, CREATE_PASS)                                         \
----------------
jamieschmeiser wrote:
> The tests for the options being empty should be moved to a separate function to facilitate expanding this feature for use with other pass instrumentation callbacks.  Right now, this is buried in here and if another callback wished to also use this freature, it might be hard to find.   Pulling the test out into an aptly named function would make it easier to find and understand.
Makes sense, done.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:467
+#include "PassRegistry.def"
+#ifndef NDEBUG
+    for (const auto &P : printBeforePasses()) {
----------------
jamieschmeiser wrote:
> This will silently give no tracing in release mode if the pass name does not exist (ie the error would not be reported and there would be no output).  Is this because of efficiency concerns?  It will only look for the names that are in the option list, which would typically be empty.  Rather than walking the string map, you could have a local stringset, add the pass names into it in the macros above if checking will be done and then use the stringset for the error checking.  I think this would be more efficient than walking the stringmap for producing the errors.
You're right, this should normally be empty so it should fine to always check. Removed the check for NDEBUG.

When --print-before/after is not empty, any minor slowdown shouldn't matter since you'd only be using --print-before/after while debugging. So I'd prefer to keep checking the map which is a bit simpler code-wise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87216