[PATCH] D100231: [NewPM] Cleanup IR printing instrumentation

[Jamie Schmeiser via Phabricator via llvm-commits]

jamieschmeiser requested changes to this revision.
jamieschmeiser added a comment.
This revision now requires changes to proceed.

Thanks for doing this.  There are a couple of comments/asserts that were missed...The only real thing is that the functionality of unwrapping Any is repeated many times... perhaps a function taking lambdas for what to do with each type of IR would be useful, probably in the header with Any?  WDYT?



================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:196
 
 /// Extracting Module out of \p IR unit. Also fills a textual description
 /// of \p IR for use in header when printing.
----------------
Remove second sentence of this comment.  It is no longer valid.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:218
     }
     assert(!Force && "Expected to have made a pair when forced.");
+    return nullptr;
----------------
"made a pair" => "a Module"


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:222
 
   if (any_isa<const Loop *>(IR)) {
     const Loop *L = any_cast<const Loop *>(IR);
----------------
This could be changed to an assert and then the llvm_unreachable can be removed.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:280
+
+  if (any_isa<const Loop *>(IR)) {
+    const Loop *L = any_cast<const Loop *>(IR);
----------------
Again, assert this and remove llvm_unreachable.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:323
+
+  if (any_isa<const Loop *>(IR)) {
+    const Loop *L = any_cast<const Loop *>(IR);
----------------
Asert and remove llvm_unreachable.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:362
 
   if (any_isa<const Loop *>(IR)) {
     const Loop *L = any_cast<const Loop *>(IR);
----------------
Assert and remove the llvm_unreachable.  The basic form of this function appears 4 or 5 times...would it make sense to have a function taking lambdas for the varying actions (in this case, calling printIR) to common up the unpacking logic associated with Any?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100231