[PATCH] D84115: [Debuginfo][Codegen] (2/7) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy).

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

A few more comments inline. I think everything in this patch works; there's still an open question in my mind of why we need to create the implicit variables so early, during instruction selection. Isn't it sufficient to leave a:

  dbg.value(0, !123, !DIExpression(DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_implicit_pointer)

In exactly the same form through the MIR phases:

  DBG_VALUE 0, $noreg, !123, !DIExpression(DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_implicit_pointer)

And then create the implicit variables at a later stage? That would avoids a number of SelectionDAG changes, plus global and fast isel too.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:86
+  // replaced by the actual value in function DwarfDebug::emitDebugLocEntry.
+  SmallVector<const DIVariable *, 8> ImplicitVars;
+
----------------
Thinking about it, would UniqueVector be a more appropriate data structure? It's more heavy weight, but contains a map for quick lookup of elements. findOrInsertImplicitVar below uses std::find which is O(n), which isn't going to scale well.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:260-261
+  if (Expr->isImplicitPointer()) {
+    const DIVariable *DVI =
+        cast<DIVariable>(MI->getDebugOperand(0).getMetadata());
+    return DbgValueLoc(Expr, DVI);
----------------
`getDebugImplicitVariable`?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1223
   if (CI->use_empty()) {
-    salvageDebugInfo(*CI);
     CI->eraseFromParent();
----------------
Not clear why this is here -- slipped in by accident? If not, is there a reason why we shouldn't be salvaging these casts? Same below.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SDNodeDbgValue.h:49
     unsigned VReg;          ///< Valid for registers.
+    DIVariable *ImpPtr;
   } u;
----------------
Doxygen for this field please


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1287-1295
+  while (Count++ < Level) {
+    if (DIDerivedType *DT = dyn_cast<DIDerivedType>(Type))
+      Type = DT->getBaseType();
+    uint32_t AlignInBits = DI.getVariable()->getAlignInBytes();
+
+    DILocalVariable *PteVar =
+        DIB.createImplicitTempVariable(Scope, Type, AlignInBits);
----------------
An illegal input to this loop would be a multi-level implicit pointer expression, and a variable type that has less indirection. IMO, there should be an assertion that this doesn't happen, to avoid it being silently accepted. Not sure where to put it though -- assert that Type is a DIDerivedType each time?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1323-1324
 
+bool SelectionDAGBuilder::handleDebugValueWrapper(const DbgValueInst &DI,
+                                                  DebugLoc dl, unsigned Order) {
+  DILocalVariable *Variable = DI.getVariable();
----------------
jmorse wrote:
> NB: this function definitely needs some comments in it as to what's going on.
Comment-ping on this :)


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

https://reviews.llvm.org/D84115