[PATCH] D88232: [DebugInfo] Handle multiple variable location operands in IR

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

mostly lgtm, some initinal comments inline, thanks!



================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2768
 
+    Value *New;
+
----------------
Can we add a comment describing what we use this for?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1225
     Instruction &VAsInst = *cast<Instruction>(V);
-    DIExpression *NewExpr = salvageDebugInfoImpl(VAsInst, Expr, StackValue);
+    // Temporary "0", awaiting real implementation.
+    DIExpression *NewExpr = salvageDebugInfoImpl(VAsInst, Expr, StackValue, 0);
----------------
So here we will be using the new metadata by salvaging dbg.values with multiple SSA values?


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1474
+    auto IsInvalidLocation = [&NewFunc](Value *Location) {
+      // Location is invalid if...
+      // ...it isn't a constant or an instruction.
----------------
Can we refactor this comments? :)


================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:263
+  auto Vs = DVI->getValues();
+  assert(Vs.size() == 1 &&
+         "expected single value for Debug Value with empty DIExpression");
----------------
Should this go into the `Verifier`, since we do not use this in the `SalvageDebugInfo` (yet) ?


================
Comment at: llvm/lib/Transforms/Utils/LCSSA.cpp:226
     // Update pre-existing debug value uses that reside outside the loop.
-    auto &Ctx = I->getContext();
     for (auto DVI : DbgValues) {
----------------
This should be different (nfc) change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88232