[PATCH] D54740: [NewPM] fixing asserts on deleted loop in -print-after-all

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: include/llvm/IR/PassInstrumentation.h:152-154
+  /// AfterPass instrumentation point - overload that takes just a \p Pass
+  /// that has been executed and does not take any IR at all. For use when
+  /// IR has been invalidated by \p Pass execution.
----------------
I think this should have a different name to indicate that it is called in a fundamentally different situation -- after an IR unit is invalidated.


================
Comment at: include/llvm/IR/PassInstrumentation.h:158-159
+    if (Callbacks)
+      for (auto &C : Callbacks->AfterPassCallbacks)
+        C(Pass.name(), llvm::Any((const IRUnitT *)nullptr));
+  }
----------------
Similarly, I think the callbacks should be different. The user can always do this binding with nullptr if that's a desirable way to model it, but generally I think different names of routines seem more appropriate.


================
Comment at: lib/Passes/StandardInstrumentations.cpp:136-137
   SmallString<20> Banner = formatv("*** IR Dump After {0} ***", PassID);
+  // TODO: handle cases when IR is nullptr (i.e. has been invalidated)
+  // but we still need to print the module due to forcePrintModuleIR.
   unwrapAndPrint(Banner, IR);
----------------
FWIW, I think the way to handle this is to bind a reference to the module into the callback so that it intrinsically has access to it to print.


================
Comment at: lib/Transforms/Scalar/LoopPassManager.cpp:47
 
-    PI.runAfterPass<Loop>(*Pass, L);
+    // do not pass deleted Loop into the instrumentation
+    if (U.skipCurrentLoop())
----------------
nit: Here and elsewhere in comments: please use proper prose with capitalized first letter and punctuation.


Repository:
  rL LLVM

https://reviews.llvm.org/D54740