[PATCH] D91722: [DebugInfo] Use variadic debug values to salvage BinOps and GEP instrs with non-const operands

[David Stenberg via Phabricator via llvm-commits]

dstenb added a comment.

Exciting!

I took the liberty to add some review comments whilst familiarizing myself with the code.



================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1369
+    if (ExprOp.getOp() == dwarf::DW_OP_LLVM_arg)
+      Result = Result <= ExprOp.getArg(0) ? ExprOp.getArg(0) + 1 : Result;
+  assert(hasAllLocationOps(Result) &&
----------------
Nit: `std::max(Result, ExprOp.getArg(0) + 1)` ?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1759-1760
+  auto *ConstInt = dyn_cast<ConstantInt>(BI->getOperand(1));
+  // Values wider than 64 bits cannot be represented within a DWARF location
+  // expression.
+  if (ConstInt && ConstInt->getBitWidth() > 64)
----------------
Should this say `DIExpression` perhaps? DWARFv5 supports binary operations on base types wider than 64 bits, but LLVM does not support such expressions.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1872
   if (auto *GEP = dyn_cast<GetElementPtrInst>(&I)) {
-    unsigned BitWidth =
-        M.getDataLayout().getIndexSizeInBits(GEP->getPointerAddressSpace());
-    // Rewrite a constant GEP into a DIExpression.
-    APInt Offset(BitWidth, 0);
-    if (GEP->accumulateConstantOffset(M.getDataLayout(), Offset)) {
-      return applyOffset(Offset.getSExtValue());
-    } else {
-      return nullptr;
-    }
+    if (getSalvageOpsForGEP(GEP, DL, CurrentLocOps, Ops, AdditionalValues))
+      return doSalvage(Ops);
----------------
Could the creation of this utility function and `getSalvageOpsForBinOp` be done as a preceding NFC refactoring commit? Just to make it slightly easier to see what the difference in behavior is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91722