[PATCH] D57694: [DebugInfo][DAG] Either salvage dangling debug info or emit Undef DBG_VALUEs

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Looks mostly good!



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1195
 
+// Make some final attempts to salvage debugging information
+void SelectionDAGBuilder::finalDebugFixup(DanglingDebugInfo &DDI) {
----------------
That comment should be in the function body or in the header.

Perhaps rename the function so it contains the word `salvage`?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1200
+  DIExpression *Expr = DDI.getDI()->getExpression();
+  auto dl = DDI.getdl();
+  unsigned SDOrder = DDI.getSDNodeOrder();
----------------
variable name & explicit type?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1221
+    // expression, bail out.
+    if (NewExpr == nullptr)
+      break;
----------------
!NewExpr


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1239
+  // couldn't be done. Place an undef DBG_VALUE at this location to terminate
+  // any earlier variable location.
+  auto Undef = UndefValue::get(Type::getInt32Ty(DDI.getDI()->getContext()));
----------------
Great!


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1240
+  // any earlier variable location.
+  auto Undef = UndefValue::get(Type::getInt32Ty(DDI.getDI()->getContext()));
+  auto SDV = DAG.getConstantDbgValue(Var, Expr, Undef, dl, SDNodeOrder);
----------------
Shouldn't we better use the type that was in the dbg.value for consistency? It makes no semantic difference, but it makes the IR less confusing to read.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1244
+
+  LLVM_DEBUG(dbgs() << "Dropping debug location info for:\n  " << DDI.getDI()
+                    << "\n");
----------------
`debug location` is usually used to refer to DILocation. Perhaps just say "debug value" instead?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:678
   /// overlapping part of variable considering the \p Expr, then this method
   /// weill drop that debug info as it isn't valid any longer.
   void dropDanglingDebugInfo(const DILocalVariable *Variable,
----------------
found a typo in the context


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h:688
+  // resolve the debuginfo to something that is represented in this DAG. If
+  // this cannot be done, produce an Undef debug value record.
+  void finalDebugFixup(DanglingDebugInfo &DDI);
----------------
`///`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57694