[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
llvm-commits at lists.llvm.org
Thu Dec 3 10:40:53 PST 2020
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
More information about the llvm-commits
mailing list