[PATCH] D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks for the patch. I have some comments inline.

Let's land debugify-each support for module and function passes before tackling other types of passes.



================
Comment at: test/DebugInfo/debugify.ll:31
+; RUN: opt -debugify-each -strip -S -o - < %s | \
+; RUN:   FileCheck %s -check-prefix=CHECK-EACH-FAIL
+
----------------
Could you move the tests for -debugify-each to a new file? This one is getting hard to follow.


================
Comment at: test/DebugInfo/debugify.ll:31
+; RUN: opt -debugify-each -strip -S -o - < %s | \
+; RUN:   FileCheck %s -check-prefix=CHECK-EACH-FAIL
+
----------------
vsk wrote:
> Could you move the tests for -debugify-each to a new file? This one is getting hard to follow.
In the new test for -debugify-each, we should test `opt -O1 -debugify-each` and check that at least one module pass and function pass are instrumented & checked.


================
Comment at: tools/opt/Debugify.cpp:65
+  addDebugifyOperand(NextLine - 1); // Original number of lines.
+  addDebugifyOperand(NextVar - 1);  // Original number of variables.
+}
----------------
We should assert that the named metadata here has exactly 2 operands, as it's not immediately clear from the way the debugify function pass is invoked that this is the case.


================
Comment at: tools/opt/Debugify.cpp:191
+void printCheckDebugifyResults(std::string Conclusion,
+                               BitVector MissingLines,
+                               BitVector MissingVars) {
----------------
These bitvectors should be passed as references to avoid copies ('const BitVector &').


================
Comment at: tools/opt/Debugify.cpp:207
+  if (Function *F = M.getFunction("llvm.dbg.value")) {
+    F->replaceAllUsesWith(UndefValue::get((Type*)F->getType()));
+    F->eraseFromParent();
----------------
Why is the explicit cast needed?


================
Comment at: tools/opt/Debugify.cpp:279
+  bool doInitialization(Module &M) override {
+    ShouldApply = (M.getNamedMetadata("llvm.dbg.cu") == nullptr);
+    if (!ShouldApply)
----------------
The extra parens are not needed here -- you can just write 'ShouldApply = !...'


================
Comment at: tools/opt/Debugify.cpp:312
+  CheckDebugifyModulePass(bool Strip = false) : ModulePass(ID) {
+    this->Strip = Strip;
+  }
----------------
You can move this to the initialization list as 'Strip(Strip)'.


================
Comment at: tools/opt/Debugify.cpp:357
+
+  CheckDebugifyFunctionPass(bool Strip = false) : FunctionPass(ID) {
+    this->Strip = Strip;
----------------
Ditto.


================
Comment at: tools/opt/opt.cpp:264
 
+class DebugifyEachPassManager : public legacy::PassManager {
+  bool DebugifyEach;
----------------
How about a more general name, like 'OptCustomPassManager'?


================
Comment at: tools/opt/opt.cpp:268
+public:
+  DebugifyEachPassManager(bool DebugifyEach) : legacy::PassManager(),
+                                               DebugifyEach(DebugifyEach) {}
----------------
The DebugifyEach flag doesn't need to be passed in here.


================
Comment at: tools/opt/opt.cpp:282
+    // Do not debugify PrinterPass
+    else if (P->getPassName() == "Print Module IR") {
+      PassManager::add(P);
----------------
Hm, this misses the 'Print Function IR' pass. There should be a less brittle way to do this. Could you add a 'bool isIRPrintingPass(Pass *P)' function to IRPrintingPasses.h? You can inspect the address of the pass ID to implement the predicate.


================
Comment at: tools/opt/opt.cpp:287
+      switch (Kind) {
+        case PT_BasicBlock:
+          PassManager::add(P);
----------------
Please use fall through to fold bodies for the identical cases together.


Repository:
  rL LLVM

https://reviews.llvm.org/D46525