Partial Reapply "[DebugInfo] Use variadic debug values to salvage BinOps and GEP instrs with non-const operands"

This is a partial reapply of the original commit and the followup commit
that were previously reverted; this reapply also includes a small fix
for a potential source of non-determinism, but also has a small change
to turn off variadic debug value salvaging, to ensure that any future
revert/reapply steps to disable and renable this feature do not risk
causing conflicts.

Differential Revision: https://reviews.llvm.org/D91722

This reverts commit 386b66b2fc297cda121a3cc8a36887a6ecbcfc68.

1 files changed, 79 insertions, 30 deletions

diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index c228df2..cfbc24c 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -1718,6 +1718,9 @@ void llvm::salvageDebugInfo(Instruction &I) {
 
 void llvm::salvageDebugInfoForDbgValues(
     Instruction &I, ArrayRef<DbgVariableIntrinsic *> DbgUsers) {
+  // This is an arbitrary chosen limit on the maximum number of values we can
+  // salvage up to in a DIArgList, used for performance reasons.
+  const unsigned MaxDebugArgs = 16;
   bool Salvaged = false;
 
   for (auto *DII : DbgUsers) {
@@ -1730,17 +1733,34 @@ void llvm::salvageDebugInfoForDbgValues(
         is_contained(DIILocation, &I) &&
         "DbgVariableIntrinsic must use salvaged instruction as its location");
     unsigned LocNo = std::distance(DIILocation.begin(), find(DIILocation, &I));
-
-    DIExpression *DIExpr =
-        salvageDebugInfoImpl(I, DII->getExpression(), StackValue, LocNo);
+    SmallVector<Value *, 4> AdditionalValues;
+    DIExpression *SalvagedExpr = salvageDebugInfoImpl(
+        I, DII->getExpression(), StackValue, LocNo, AdditionalValues);
 
     // salvageDebugInfoImpl should fail on examining the first element of
     // DbgUsers, or none of them.
-    if (!DIExpr)
+    if (!SalvagedExpr)
       break;
 
     DII->replaceVariableLocationOp(&I, I.getOperand(0));
-    DII->setExpression(DIExpr);
+    if (AdditionalValues.empty()) {
+      DII->setExpression(SalvagedExpr);
+    } else if (isa<DbgValueInst>(DII) &&
+               DII->getNumVariableLocationOps() + AdditionalValues.size() <=
+                   MaxDebugArgs) {
+      // TODO: Uncomment the line below and delete the two beneath it to enable
+      // salvaging of dbg.values with multiple location operands.
+      // DII->addVariableLocationOps(AdditionalValues, SalvagedExpr);
+      Value *Undef = UndefValue::get(I.getOperand(0)->getType());
+      DII->replaceVariableLocationOp(I.getOperand(0), Undef);
+    } else {
+      // Do not salvage using DIArgList for dbg.addr/dbg.declare, as it is
+      // currently only valid for stack value expressions.
+      // Also do not salvage if the resulting DIArgList would contain an
+      // unreasonably large number of values.
+      Value *Undef = UndefValue::get(I.getOperand(0)->getType());
+      DII->replaceVariableLocationOp(I.getOperand(0), Undef);
+    }
     LLVM_DEBUG(dbgs() << "SALVAGE: " << *DII << '\n');
     Salvaged = true;
   }
@@ -1755,12 +1775,27 @@ void llvm::salvageDebugInfoForDbgValues(
 }
 
 bool getSalvageOpsForGEP(GetElementPtrInst *GEP, const DataLayout &DL,
-                         SmallVectorImpl<uint64_t> &Opcodes) {
+                         uint64_t CurrentLocOps,
+                         SmallVectorImpl<uint64_t> &Opcodes,
+                         SmallVectorImpl<Value *> &AdditionalValues) {
   unsigned BitWidth = DL.getIndexSizeInBits(GEP->getPointerAddressSpace());
-  // Rewrite a constant GEP into a DIExpression.
+  // Rewrite a GEP into a DIExpression.
+  MapVector<Value *, APInt> VariableOffsets;
   APInt ConstantOffset(BitWidth, 0);
-  if (!GEP->accumulateConstantOffset(DL, ConstantOffset))
+  if (!GEP->collectOffset(DL, BitWidth, VariableOffsets, ConstantOffset))
     return false;
+  if (!VariableOffsets.empty() && !CurrentLocOps) {
+    Opcodes.insert(Opcodes.begin(), {dwarf::DW_OP_LLVM_arg, 0});
+    CurrentLocOps = 1;
+  }
+  for (auto Offset : VariableOffsets) {
+    AdditionalValues.push_back(Offset.first);
+    assert(Offset.second.isStrictlyPositive() &&
+           "Expected strictly positive multiplier for offset.");
+    Opcodes.append({dwarf::DW_OP_LLVM_arg, CurrentLocOps++, dwarf::DW_OP_constu,
+                    Offset.second.getZExtValue(), dwarf::DW_OP_mul,
+                    dwarf::DW_OP_plus});
+  }
   DIExpression::appendOffset(Opcodes, ConstantOffset.getSExtValue());
   return true;
 }
@@ -1795,23 +1830,35 @@ uint64_t getDwarfOpForBinOp(Instruction::BinaryOps Opcode) {
   }
 }
 
-bool getSalvageOpsForBinOp(BinaryOperator *BI,
-                           SmallVectorImpl<uint64_t> &Opcodes) {
-  // Rewrite binary operations with constant integer operands.
+bool getSalvageOpsForBinOp(BinaryOperator *BI, uint64_t CurrentLocOps,
+                           SmallVectorImpl<uint64_t> &Opcodes,
+                           SmallVectorImpl<Value *> &AdditionalValues) {
+  // Handle binary operations with constant integer operands as a special case.
   auto *ConstInt = dyn_cast<ConstantInt>(BI->getOperand(1));
-  if (!ConstInt || ConstInt->getBitWidth() > 64)
+  // Values wider than 64 bits cannot be represented within a DIExpression.
+  if (ConstInt && ConstInt->getBitWidth() > 64)
     return false;
-  uint64_t Val = ConstInt->getSExtValue();
+
   Instruction::BinaryOps BinOpcode = BI->getOpcode();
-  // Add or Sub Instructions with a constant operand can potentially be
-  // simplified.
-  if (BinOpcode == Instruction::Add || BinOpcode == Instruction::Sub) {
-    uint64_t Offset = BinOpcode == Instruction::Add ? Val : -int64_t(Val);
-    DIExpression::appendOffset(Opcodes, Offset);
-    return true;
+  // Push any Constant Int operand onto the expression stack.
+  if (ConstInt) {
+    uint64_t Val = ConstInt->getSExtValue();
+    // Add or Sub Instructions with a constant operand can potentially be
+    // simplified.
+    if (BinOpcode == Instruction::Add || BinOpcode == Instruction::Sub) {
+      uint64_t Offset = BinOpcode == Instruction::Add ? Val : -int64_t(Val);
+      DIExpression::appendOffset(Opcodes, Offset);
+      return true;
+    }
+    Opcodes.append({dwarf::DW_OP_constu, Val});
+  } else {
+    if (!CurrentLocOps) {
+      Opcodes.append({dwarf::DW_OP_LLVM_arg, 0});
+      CurrentLocOps = 1;
+    }
+    Opcodes.append({dwarf::DW_OP_LLVM_arg, CurrentLocOps});
+    AdditionalValues.push_back(BI->getOperand(1));
   }
-  // Add constant int operand to expression stack.
-  Opcodes.append({dwarf::DW_OP_constu, Val});
 
   // Add salvaged binary operator to expression stack, if it has a valid
   // representation in a DIExpression.
@@ -1823,9 +1870,11 @@ bool getSalvageOpsForBinOp(BinaryOperator *BI,
   return true;
 }
 
-DIExpression *llvm::salvageDebugInfoImpl(Instruction &I,
-                                         DIExpression *SrcDIExpr,
-                                         bool WithStackValue, unsigned LocNo) {
+DIExpression *
+llvm::salvageDebugInfoImpl(Instruction &I, DIExpression *SrcDIExpr,
+                           bool WithStackValue, unsigned LocNo,
+                           SmallVectorImpl<Value *> &AdditionalValues) {
+  uint64_t CurrentLocOps = SrcDIExpr->getNumLocationOperands();
   auto &M = *I.getModule();
   auto &DL = M.getDataLayout();
 
@@ -1839,7 +1888,7 @@ DIExpression *llvm::salvageDebugInfoImpl(Instruction &I,
   };
 
   // initializer-list helper for applying operators to the source DIExpression.
-  auto applyOps = [&](ArrayRef<uint64_t> Opcodes) -> DIExpression * {
+  auto applyOps = [&](ArrayRef<uint64_t> Opcodes) {
     SmallVector<uint64_t, 8> Ops(Opcodes.begin(), Opcodes.end());
     return doSalvage(Ops);
   };
@@ -1865,15 +1914,15 @@ DIExpression *llvm::salvageDebugInfoImpl(Instruction &I,
 
   SmallVector<uint64_t, 8> Ops;
   if (auto *GEP = dyn_cast<GetElementPtrInst>(&I)) {
-    if (getSalvageOpsForGEP(GEP, DL, Ops))
+    if (getSalvageOpsForGEP(GEP, DL, CurrentLocOps, Ops, AdditionalValues))
       return doSalvage(Ops);
   } else if (auto *BI = dyn_cast<BinaryOperator>(&I)) {
-    if (getSalvageOpsForBinOp(BI, Ops))
+    if (getSalvageOpsForBinOp(BI, CurrentLocOps, Ops, AdditionalValues))
       return doSalvage(Ops);
   }
-    // *Not* to do: we should not attempt to salvage load instructions,
-    // because the validity and lifetime of a dbg.value containing
-    // DW_OP_deref becomes difficult to analyze. See PR40628 for examples.
+  // *Not* to do: we should not attempt to salvage load instructions,
+  // because the validity and lifetime of a dbg.value containing
+  // DW_OP_deref becomes difficult to analyze. See PR40628 for examples.
   return nullptr;
 }