[PATCH] D94388: [Coroutine][DebugInfo] Enhance the ability of coroutine to debug parameters under O2

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1103
   DominatorTree DT(*CB->getFunction());
+  // copy dbg from entry block if the dbg intrinsic existed
+  std::unique_ptr<DIBuilder> DIB = std::make_unique<DIBuilder>(*M, false);
----------------
Nit: According to the LLVM coding style this should be a full sentence:
`// Copy debug intrinsics from the entry block if one exists.`


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1104
+  // copy dbg from entry block if the dbg intrinsic existed
+  std::unique_ptr<DIBuilder> DIB = std::make_unique<DIBuilder>(*M, false);
 
----------------
Why not just
`DIBuilder DIB(*M, false)`?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1147
+    SmallVector<DbgVariableIntrinsic *, 8> DbgUsers;
+    // find all dbg user
+    findDbgUsers(DbgUsers, Def);
----------------
This comment is redundant with the function name, I would just skip it.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1159
 
+      // Get debug intrinsic for argument
+      auto Iter = llvm::find_if(DbgUsers, [](DbgVariableIntrinsic *Dbg) {
----------------
`.` at the end


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1162
+        DILocalVariable *Variable = Dbg->getVariable();
+        return Variable->isParameter() && !Variable->isArtificial();
+      });
----------------
why filter out artificial arguments? This will filter out the `this` pointer for example.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1166
+      // if we get multiple results, just pick one
+      if (Iter != DbgUsers.end())
+        DebugInstrinsic = *Iter;
----------------
Why not handle all of them in loop? That would give the frontend more freedom.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1220
+
+        if (DIB && DebugInstrinsic) {
+            auto *CurVar = DebugInstrinsic->getVariable();
----------------
Aren't we unconditionally create DIB above?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1220
+
+        if (DIB && DebugInstrinsic) {
+            auto *CurVar = DebugInstrinsic->getVariable();
----------------
aprantl wrote:
> Aren't we unconditionally create DIB above?
and then do `for (DebugIntrinsic : DebugIntrinsics)` here


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1225
+
+            // try to get scope form gep or use original one
+            Instruction *InstGEP = cast<Instruction>(GEP);
----------------
 // Try to get the scope from the gep or use the original one.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1228
+            DIExpression *NewExpr = DIExpression::append(
+                DebugInstrinsic->getExpression(), dwarf::DW_OP_deref);
+            Value *NewValue = InstGEP;
----------------
Why do we need to do this manually? Shouldn't salvageDebugInfo be able to translate a GEP into a combination of DW_OP_plus and DW_OP_deref?


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:99
+        do {
+          // 1. if user is empty, we can safe to remove it
+          if (MetaVal->user_empty()) {
----------------
This statement is not correct.

Imagine the following code:
```
void f(int i) {
  if (i==42) ...;
 ...
}
```

in IR:

```
if.then:
  dbg.value(i32 42, DILocalVariable("i"), DIExpression())
  // optimized out
  br %if.end
if.end:
  // Let's assume "i" has been optimized away for here on.
  dbg.value(i32 undef, DILocalVariable("i"), DIExpression())
```

If we remove the undef dbg.value, the debug info will make it look as if the value for i was unconditionally 42 on all paths, which would be wrong in all paths where the branch wasn't taken.

My point is, it is never safe to remove a debug intrinsic. We may be able to remove them inside of the CoroFrame code based on special knowledge that we have there, but it can't be a general helper function in Local.cpp, because what the function does isn't generally correct.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:149
+  // remove dbgs
+  DeleteUselessDbgInfo(BBs);
+
----------------
We can't do this //here//, unfortunately.


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

https://reviews.llvm.org/D94388