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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks! At a high level this is looking good. I have more comments inline which should help with code reuse. Currently there is some extra duplicated logic for function passes, and this should be consolidated for easier maintenance.



================
Comment at: tools/opt/Debugify.cpp:209
+  if (Function *F = M.getFunction("llvm.dbg.value")) {
+    F->replaceAllUsesWith(UndefValue::get(F->getType()));
+    F->eraseFromParent();
----------------
Why is this RAUW needed at all? Should StripDebugInfo handle this for you?


================
Comment at: tools/opt/Debugify.cpp:242
+                                           : std::string("PASS"));
+  printCheckDebugifyResults(Conclusion, MissingLines, MissingVars);
 }
----------------
You shouldn't need to duplicate any logic for setting up these BitVectors, and you shouldn't need a separate printCheckDebugifyResults() function.

Please reuse the same logic for checking debugify metadata in module and function passes. E.g:
```
void checkDebugifyMetadata(Module &M, iterator_range<Module::iterator> Functions) {
  // Get the number of lines / vars from the module's named metadata, set up the bit vectors, etc.
  for (Function &F : Functions) {
    // Check for missing lines / vars.
  }
  // Print the results.
}

// In the FunctionPass helper:
checkDebugifyMetadata(M, make_range(F.getIterator(), std::next(F.getIterator()))); //< Just check "F".

// In the ModulePass helper:
checkDebugifyMetadata(M, M.functions());
```

If in the future we add a debugify wrapper for BasicBlock passes, we can simply add a 'BasicBlock *' parameter to 'checkDebugifyMetadata'.


================
Comment at: tools/opt/Debugify.cpp:262
+struct DebugifyFunctionPass : public FunctionPass {
+  bool runOnFunction(Function &F) override {
+    if (!ShouldApply)
----------------
Per my previous comments, I'd suggest making two changes here:
- Move all of the logic to a helper to make a future port to the new PM easier.
- Make `applyDebugifyMetadata` accept a range of functions to visit (`iterator_range<Module::iterator>`), so there is only one call to createCompileUnit, etc. in this file.


================
Comment at: tools/opt/Debugify.cpp:281
+  bool doInitialization(Module &M) override {
+    ShouldApply = M.getNamedMetadata("llvm.dbg.cu") == nullptr;
+    if (!ShouldApply)
----------------
I don't think we should compute and store ShouldApply in the doInitialization() step, because it happens just once per pass. It should be done in the `applyDebugifyMetadata` function.


================
Comment at: tools/opt/Debugify.cpp:324
+struct CheckDebugifyFunctionPass : public FunctionPass {
+  bool runOnFunction(Function &F) override {
+    Module *M = F.getParent();
----------------
Please move all of the actual logic here into a helper function, to simplify a future port to the new pass manager.


================
Comment at: tools/opt/PassPrinters.h:67
 
+llvm::ModulePass *createStripDbgInfoPass();
+
----------------
I think this is dead code.


================
Comment at: tools/opt/opt.cpp:264
 
+class OptCustomPassManager : public legacy::PassManager {
+public:
----------------
You can use an idiom here to reduce repetition and simplify the code:
```
using super = legacy::PassManager;
```


================
Comment at: tools/opt/opt.cpp:269
+  void add(Pass *P) override {
+    if (!DebugifyEach) {
+      PassManager::add(P);
----------------
Please use an early-return for the cases (outside of the switch) where we don't insert extra passes. E.g:
```
bool WrapWithDebugify = DebugifyEach && !P->getAsImmutablePass() && !isIRPrintingPass(P);
if (!WrapWithDebugify) {
  super::add(P);
  return;
}
```


================
Comment at: tools/opt/opt.cpp:284
+      // TODO: Implement Debugify for BasicBlockPass, RegionPass, LoopPass and
+      // CallGraphSCCPass.
+      switch (Kind) {
----------------
We can safely skip RegionPass and CGSCCPass, as they are just used for structurize-cfg, the old inliner, and 2-3 other passes. I don't think it's worth adding the complexity to support this, so we can leave it out of the TODO.


================
Comment at: tools/opt/opt.cpp:626
 
-  if (EnableDebugify)
-    Passes.add(createDebugifyPass());
+  if (EnableDebugify && !DebugifyEach)
+    Passes.add(createDebugifyModulePass());
----------------
This conditional shouldn't be written out twice. Could you introduce 'bool AddOneTimeDebugifyPasses = EnableDebugify && !DebugifyEach'?


Repository:
  rL LLVM

https://reviews.llvm.org/D46525