[PATCH] D97673: [RFC] [[Coroutine] [Debug] Salvage dbg.values

[Xun Li via Phabricator via llvm-commits]

lxfind added a comment.

The test is insufficient. I would like to see that we cover dbg.value used on different cases: parameters, allocas, spills.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1282
       // reload.
       U->replaceUsesOfWith(Def, CurrentReload);
     }
----------------
Why doesn't replaceUsesOfWith work? Could you double check that replaceUsesOfWith doesn't work for DbgValueInst?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1347-1352
+        DIBuilder(*Alloca->getModule(),
+                  /*AllowUnresolved*/ false)
+            .insertDbgValueIntrinsic(G, DVI->getVariable(),
+                                     DVI->getExpression(), DVI->getDebugLoc(),
+                                     DVI);
+        DVI->eraseFromParent();
----------------
If replaceUsesOfWith works for DbgValueInst, then I think you can just append all DIs into UsersToUpdate above.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:1607
       }
+      if (auto *DVI = dyn_cast<DbgValueInst>(U)) {
+        DVI->replaceVariableLocationOp(Def, CurrentMaterialization);
----------------
Similarly, if replaceUsesOfWith works for DbgValueInst, this isn't necessary


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2333-2345
+  // We only collect dbg.values for who would be in the Frame.
+  // So these dbg.values wouldn't affect the layout of the Frame.
+  for (auto *Def : FrameData.getAllDefs()) {
+    // We would handle alloca specially.
+    if (isa<AllocaInst>(Def))
+      continue;
+    SmallVector<DbgValueInst *, 16> DVIs;
----------------
I think it's cleaner to move this code into the loop above.
After the check of all users, you can check if `FrameData.Spills` contains `&I`, and if so, do this DbgValues thing.


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

https://reviews.llvm.org/D97673