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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks for working on this @tyb0807, it looks really close :). There are just a few more minor things I'd like addressed.



================
Comment at: test/DebugInfo/debugify-each.ll:1
+; RUN: opt -debugify-each -O1 -S -print-after=called-value-propagation -o - < %s 2> %t
+; FileCheck < %t %s
----------------
At a high level, we don't want to test the debugify logic in this file, because we already have a test for that. Instead, we want to check that -debugify-each is running after each pass.

Let's minimize this test:

- Please remove the checks for what the inserted debug info metadata looks like.
- We just need two functions, e.g "foo" and "bar", which both only contain "ret void".
- Add RUN lines for: "-debugify-each -O{1,2,3}", and "-enable-debugify -debugify-each -sroa -sccp". All of these separate test runs should share the exact same FileCheck "CHECK" commands. Specifically, we expect the module & function debugify passes to run at least twice. So you could write:

```
; Verify that the module & function (check-)debugify passes run at least twice.

; CHECK: CheckModuleDebugify: PASS
; CHECK: CheckFunctionDebugify: PASS
; CHECK: CheckFunctionDebugify: PASS

; CHECK: CheckModuleDebugify: PASS
; CHECK: CheckFunctionDebugify: PASS
; CHECK: CheckFunctionDebugify: PASS
```

These should be the only CHECK lines you need.


================
Comment at: tools/opt/Debugify.cpp:44
+                           iterator_range<Module::iterator> Functions,
+                           std::string Banner) {
   // Skip modules with debug info.
----------------
Could you use a StringRef here to avoid copying the std::string?


================
Comment at: tools/opt/Debugify.cpp:56
   DenseMap<uint64_t, DIType *> TypeCache;
-  auto getCachedDIType = [&](Type *Ty) -> DIType * {
+  auto getCachedDIType = [&] (Type *Ty) -> DIType * {
     uint64_t Size =
----------------
Could you run clang-format? There are some whitespace-only changes which complicate the diff.


================
Comment at: tools/opt/Debugify.cpp:129
+                           iterator_range<Module::iterator> Functions,
+                           std::string Banner,
+                           bool Strip) {
----------------
Ditto, please use StringRef.


================
Comment at: tools/opt/Debugify.cpp:160
       auto DL = I.getDebugLoc();
-      if (DL) {
+      if (DL && MissingLines.size() > (DL.getLine() - 1)) {
         MissingLines.reset(DL.getLine() - 1);
----------------
We should only need special handling for "line 0", as that refers to a compiler-generated "ambiguous" location. I.e, `if (DL && DL.getLine() != 0) { reset(...) }`.

We don't need need any other range checks, because an assertion in BitVector should catch out-of-bounds accesses. This means we can omit the "if (DL)" which follows.



================
Comment at: tools/opt/Debugify.cpp:183
       (void)to_integer(DVI->getVariable()->getName(), Var, 10);
-      assert(Var <= OriginalNumVars && "Unexpected name for DILocalVariable");
+      assert(Var <= MissingVars.size() && "Unexpected name for DILocalVariable");
       MissingVars.reset(Var - 1);
----------------
As this change doesn't change any behavior, let's keep the previous version to minimize the diff.


================
Comment at: tools/opt/Debugify.cpp:207
+    if (Function *F = M.getFunction("llvm.dbg.value"))
+      F->eraseFromParent();
+    return true;
----------------
I still don't understand why we need to erase the declaration of llvm.dbg.value() from the module. Shouldn't StripDebugInfo() do this for us? If not, we should just fix StripDebugInfo() in a follow-up patch.


================
Comment at: tools/opt/Debugify.cpp:230
+
+/// FunctionPass for attaching synthetic debug info to everything, used with the
+/// legacy module pass manager.
----------------
Please replace "to everything" with "to instructions within a single function".


================
Comment at: tools/opt/Debugify.cpp:233
+struct DebugifyFunctionPass : public FunctionPass {
+  bool runOnFunction(Function &F) override {
+    return applyDebugifyMetadata(*(F.getParent()),
----------------
Just a nit, but this could be easier to read if it's split up into logically smaller pieces, e.g:
```
Module &M = *F.getParent();
auto FuncIt = F.getIterator();
return apply...(M, make_range(FuncIt, std::next(FuncIt)), ...);
```


================
Comment at: tools/opt/Debugify.cpp:271
+    return checkDebugifyMetadata(*(F.getParent()),
+                     make_range(F.getIterator(), std::next(F.getIterator())),
+                     "CheckFunctionDebugify: ",
----------------
Same nitpick about simplifying this callsite.


================
Comment at: tools/opt/opt.cpp:267
+  using super = legacy::PassManager;
+  OptCustomPassManager() : super() {}
+
----------------
The implicit default constructor here calls the base constructor. You can just leave this line out -- the compiler does it for you.


Repository:
  rL LLVM

https://reviews.llvm.org/D46525