Streamline the API of salvageDebugInfoImpl (NFC)

[aprantl]

Summary

This patch refactors / simplifies salvageDebugInfoImpl(). The goal here is to simplify the implementation of coro::salvageDebugInfo() in a followup patch.

1. Change the return value to I.getOperand(0). Currently users of salvageDebugInfoImpl() assume that the first operand is I.getOperand(0). This patch makes this information explicit. A nice side-effect of this change is that it allows us to salvage expressions such as add i8 1, %a in the future.
2. Factor out the creation of a DIExpression and return an array of DIExpression operations instead. This change allows users that call salvageDebugInfoImpl() in a loop to avoid the costly creation of temporary DIExpressions and to defer the creation of a DIExpression until the end.

This patch does not change any functionality.

[aprantl]

Fix a use-after-free of Op0 introduced in the previous revision and remove the redundant check for Ops.empty().

[Orlando]

LGTM, I just have a few small inline questions and nits.

llvm/include/llvm/Transforms/Utils/Local.h

296

Should this be nullptr rather than false?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1285–1286

When is Expr nullptr here (and for the other salvageDebugInfoImpl calls where you make a similar check)? I may be missing something but it looks like the Expr? check wasn't necessary before.

1288–1292

It might make sense to move the if (!V) check above to underneath this comment? It doesn't matter much though.

llvm/lib/Transforms/Utils/Local.cpp

1758–1759

Do we still need the SalvagedExpr check here - I'm not sure it will ever be nullptr now?

1759

How did you choose the SmallVector size value of 16? I don't think it matters for the review, I am just curious.

[aprantl]

llvm/include/llvm/Transforms/Utils/Local.h

296

Thanks!

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1285–1286

I only threw in the check because DbgIntrinsicInst::getExpression() returns a DIExpression* and I wasn't sure whether that meant the expression is guaranteed to be non-null. I wish LLVM would use references in APIs to make this unambiguous. It's probably not necessary, and I'm happy to remove it.

1288–1292

Yes.

[aprantl]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

1285–1286

Actually, DIBuilder::insertDbgValueIntrinsic() doesn't prevent a language frontend from creating a dbg.value with a nullptr DIExpression. And Verifier accepts it, too. If we wanted to make this guarantee then we'd need to enforce it in the Verifier first.

llvm/lib/Transforms/Utils/Local.cpp

1758–1759

Same comment as above, there is unfortunately no guarantee that the expressions from dbg.value intrinsics are non-null.

1759

I picked a reasonably large, round number without any statistics to back up my choice :-)

llvm/include/llvm/Transforms/Utils/Local.h

 
 /// Implementation of salvageDebugInfo, applying only to instructions in
 /// \p Insns, rather than all debug users from findDbgUsers( \p I).
 /// Returns true if any debug users were updated.
 /// Mark undef if salvaging cannot be completed.
 void salvageDebugInfoForDbgValues(Instruction &I,
                                   ArrayRef<DbgVariableIntrinsic *> Insns);
 
-/// Given an instruction \p I and DIExpression \p DIExpr operating on it, write
-/// the effects of \p I into the returned DIExpression, or return nullptr if
-/// it cannot be salvaged. \p StackVal: whether DW_OP_stack_value should be
-/// appended to the expression. \p LocNo: the index of the location operand to
-/// which \p I applies, should be 0 for debug info without a DIArgList.
-DIExpression *salvageDebugInfoImpl(Instruction &I, DIExpression *DIExpr,
-                                   bool StackVal, unsigned LocNo,
-                                   SmallVectorImpl<Value *> &AdditionalValues);
+/// Given an instruction \p I and DIExpression \p DIExpr operating on
+/// it, append the effects of \p I to the DIExpression operand list
+/// \p Ops, or return \p nullptr if it cannot be salvaged.
+/// \p CurrentLocOps is the number of SSA values referenced by the
+/// incoming \p Ops.  \return the first non-constant operand
+/// implicitly referred to by Ops. If \p I references more than one
+/// non-constant operand, any additional operands are added to
+/// \p AdditionalValues.
+///
+/// \example
+////
+///   I = add %a, i32 1
+///
+///   Return = %a
+///   Ops = llvm::dwarf::DW_OP_lit1 llvm::dwarf::DW_OP_add
+///
+///   I = add %a, %b
+///
+///   Return = %a
+///   Ops = llvm::dwarf::DW_OP_LLVM_arg0 llvm::dwarf::DW_OP_add
+///   AdditionalValues = %b
+Value *salvageDebugInfoImpl(Instruction &I, uint64_t CurrentLocOps,
+                            SmallVectorImpl<uint64_t> &Ops,
 
 /// Point debug users of \p From to \p To or salvage them. Use this function
 /// only when replacing all uses of \p From with \p To, with a guarantee that
 /// \p From is going to be deleted.
 ///
 /// Follow these rules to prevent use-before-def of \p To:
 ///   . If \p To is a linked Instruction, set \p DomPoint to \p To.
 ///   . If \p To is an unlinked Instruction, set \p DomPoint to the Instruction

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (handleDebugValue(V, Var, Expr, DL, InstDL, SDOrder, /*IsVariadic=*/false))
     return;
 
   // Attempt to salvage back through as many instructions as possible. Bail if
   // a non-instruction is seen, such as a constant expression or global
   // variable. FIXME: Further work could recover those too.
   while (isa<Instruction>(V)) {
     Instruction &VAsInst = *cast<Instruction>(V);
     // Temporary "0", awaiting real implementation.
+    SmallVector<uint64_t, 16> Ops;
     SmallVector<Value *, 4> AdditionalValues;
-    DIExpression *SalvagedExpr =
-        salvageDebugInfoImpl(VAsInst, Expr, StackValue, 0, AdditionalValues);
-
+    V = salvageDebugInfoImpl(VAsInst, Expr->getNumLocationOperands(), Ops,
+                             AdditionalValues);
     // If we cannot salvage any further, and haven't yet found a suitable debug
     // expression, bail out.
+    if (!V)
+      break;
+
     // TODO: If AdditionalValues isn't empty, then the salvage can only be
     // represented with a DBG_VALUE_LIST, so we give up. When we have support
     // here for variadic dbg_values, remove that condition.
-    if (!SalvagedExpr || !AdditionalValues.empty())
+    if (!AdditionalValues.empty())
       break;
 
     // New value and expr now represent this debuginfo.
-    V = VAsInst.getOperand(0);
-    Expr = SalvagedExpr;
+    Expr = DIExpression::appendOpsToArg(Expr, Ops, 0, StackValue);
 
     // Some kind of simplification occurred: check whether the operand of the
     // salvaged debug expression can be encoded in this DAG.
     if (handleDebugValue(V, Var, Expr, DL, InstDL, SDOrder,
                          /*IsVariadic=*/false)) {
       LLVM_DEBUG(dbgs() << "Salvaged debug location info for:\n  "
                         << DDI.getDI() << "\nBy stripping back to:\n  " << V);
       return;

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

if (auto *LdInst = dyn_cast<LoadInst>(Storage)) {
       // last direct load from an alloca is necessary.  This condition
       // effectively drops the *last* DW_OP_deref in the expression.
       if (!OutermostLoad)
         Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore);
       OutermostLoad = false;
     } else if (auto *StInst = dyn_cast<StoreInst>(Storage)) {
       Storage = StInst->getOperand(0);
     } else if (auto *GEPInst = dyn_cast<GetElementPtrInst>(Storage)) {
-      SmallVector<Value *> AdditionalValues;
-      DIExpression *SalvagedExpr = llvm::salvageDebugInfoImpl(
-          *GEPInst, Expr,
-          /*WithStackValue=*/false, 0, AdditionalValues);
+      SmallVector<uint64_t, 16> Ops;
+      SmallVector<Value *, 0> AdditionalValues;
+      Storage = llvm::salvageDebugInfoImpl(
+          *GEPInst, Expr ? Expr->getNumLocationOperands() : 0, Ops,
+          AdditionalValues);
+      if (!Storage)
+        break;
       // Debug declares cannot currently handle additional location
       // operands.
-      if (!SalvagedExpr || !AdditionalValues.empty())
+      if (!AdditionalValues.empty())
         break;
-      Expr = SalvagedExpr;
-      Storage = GEPInst->getOperand(0);
+      Expr = DIExpression::appendOpsToArg(Expr, Ops, 0, /*StackValue*/ false);
     } else if (auto *BCInst = dyn_cast<llvm::BitCastInst>(Storage))
       Storage = BCInst->getOperand(0);
     else
       break;
   }
   if (!Storage)
     return;
 

llvm/lib/Transforms/Utils/Local.cpp

for (auto *DII : DbgUsers) {
     assert(
         is_contained(DIILocation, &I) &&
         "DbgVariableIntrinsic must use salvaged instruction as its location");
     SmallVector<Value *, 4> AdditionalValues;
     // `I` may appear more than once in DII's location ops, and each use of `I`
     // must be updated in the DIExpression and potentially have additional
     // values added; thus we call salvageDebugInfoImpl for each `I` instance in
     // DIILocation.
+    Value *Op0 = nullptr;
     DIExpression *SalvagedExpr = DII->getExpression();
     auto LocItr = find(DIILocation, &I);
     while (SalvagedExpr && LocItr != DIILocation.end()) {
+      SmallVector<uint64_t, 16> Ops;
       unsigned LocNo = std::distance(DIILocation.begin(), LocItr);
-      SalvagedExpr = salvageDebugInfoImpl(I, SalvagedExpr, StackValue, LocNo,
-                                          AdditionalValues);
+      uint64_t CurrentLocOps = SalvagedExpr->getNumLocationOperands();
+      Op0 = salvageDebugInfoImpl(I, CurrentLocOps, Ops, AdditionalValues);
+      if (!Op0)
+        break;
+      SalvagedExpr =
+          DIExpression::appendOpsToArg(SalvagedExpr, Ops, LocNo, StackValue);
       LocItr = std::find(++LocItr, DIILocation.end(), &I);
     }
     // salvageDebugInfoImpl should fail on examining the first element of
     // DbgUsers, or none of them.
-    if (!SalvagedExpr)
+    if (!Op0)
       break;
 
-    DII->replaceVariableLocationOp(&I, I.getOperand(0));
+    DII->replaceVariableLocationOp(&I, Op0);
     if (AdditionalValues.empty()) {
       DII->setExpression(SalvagedExpr);
     } else if (isa<DbgValueInst>(DII) &&
                DII->getNumVariableLocationOps() + AdditionalValues.size() <=
                    MaxDebugArgs) {
       DII->addVariableLocationOps(AdditionalValues, SalvagedExpr);
     } else {
       // Do not salvage using DIArgList for dbg.addr/dbg.declare, as it is
if (Salvaged)
     return;
 
   for (auto *DII : DbgUsers) {
     Value *Undef = UndefValue::get(I.getType());
     DII->replaceVariableLocationOp(&I, Undef);
   }
 }
 
-bool getSalvageOpsForGEP(GetElementPtrInst *GEP, const DataLayout &DL,
+Value *getSalvageOpsForGEP(GetElementPtrInst *GEP, const DataLayout &DL,
                            uint64_t CurrentLocOps,
                            SmallVectorImpl<uint64_t> &Opcodes,
                            SmallVectorImpl<Value *> &AdditionalValues) {
   unsigned BitWidth = DL.getIndexSizeInBits(GEP->getPointerAddressSpace());
   // Rewrite a GEP into a DIExpression.
   MapVector<Value *, APInt> VariableOffsets;
   APInt ConstantOffset(BitWidth, 0);
   if (!GEP->collectOffset(DL, BitWidth, VariableOffsets, ConstantOffset))
-    return false;
+    return nullptr;
   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;
+  return GEP->getOperand(0);
 }
 
 uint64_t getDwarfOpForBinOp(Instruction::BinaryOps Opcode) {
   switch (Opcode) {
   case Instruction::Add:
     return dwarf::DW_OP_plus;
   case Instruction::Sub:
     return dwarf::DW_OP_minus;
uint64_t getDwarfOpForBinOp(Instruction::BinaryOps Opcode) {
   case Instruction::AShr:
     return dwarf::DW_OP_shra;
   default:
     // TODO: Salvage from each kind of binop we know about.
     return 0;
   }
 }
 
-bool getSalvageOpsForBinOp(BinaryOperator *BI, uint64_t CurrentLocOps,
+Value *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));
   // Values wider than 64 bits cannot be represented within a DIExpression.
   if (ConstInt && ConstInt->getBitWidth() > 64)
-    return false;
+    return nullptr;
 
   Instruction::BinaryOps BinOpcode = BI->getOpcode();
   // 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;
+      return BI->getOperand(0);
     }
     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 salvaged binary operator to expression stack, if it has a valid
   // representation in a DIExpression.
   uint64_t DwarfBinOp = getDwarfOpForBinOp(BinOpcode);
   if (!DwarfBinOp)
-    return false;
+    return nullptr;
   Opcodes.push_back(DwarfBinOp);
-
-  return true;
+  return BI->getOperand(0);
 }
 
-DIExpression *
-llvm::salvageDebugInfoImpl(Instruction &I, DIExpression *SrcDIExpr,
-                           bool WithStackValue, unsigned LocNo,
+Value *llvm::salvageDebugInfoImpl(Instruction &I, uint64_t CurrentLocOps,
+                                  SmallVectorImpl<uint64_t> &Ops,
                                   SmallVectorImpl<Value *> &AdditionalValues) {
-  uint64_t CurrentLocOps = SrcDIExpr->getNumLocationOperands();
   auto &M = *I.getModule();
   auto &DL = M.getDataLayout();
 
-  // Apply a vector of opcodes to the source DIExpression.
-  auto doSalvage = [&](SmallVectorImpl<uint64_t> &Ops) -> DIExpression * {
-    DIExpression *DIExpr = SrcDIExpr;
-    if (!Ops.empty()) {
-      DIExpr = DIExpression::appendOpsToArg(DIExpr, Ops, LocNo, WithStackValue);
-    }
-    return DIExpr;
-  };
-
-  // initializer-list helper for applying operators to the source DIExpression.
-  auto applyOps = [&](ArrayRef<uint64_t> Opcodes) {
-    SmallVector<uint64_t, 8> Ops(Opcodes.begin(), Opcodes.end());
-    return doSalvage(Ops);
-  };
-
   if (auto *CI = dyn_cast<CastInst>(&I)) {
+    Value *FromValue = CI->getOperand(0);
     // No-op casts are irrelevant for debug info.
-    if (CI->isNoopCast(DL))
-      return SrcDIExpr;
+    if (CI->isNoopCast(DL)) {
+      return FromValue;
+    }
 
     Type *Type = CI->getType();
     // Casts other than Trunc, SExt, or ZExt to scalar types cannot be salvaged.
     if (Type->isVectorTy() ||
         !(isa<TruncInst>(&I) || isa<SExtInst>(&I) || isa<ZExtInst>(&I)))
       return nullptr;
 
-    Value *FromValue = CI->getOperand(0);
     unsigned FromTypeBitSize = FromValue->getType()->getScalarSizeInBits();
     unsigned ToTypeBitSize = Type->getScalarSizeInBits();
 
-    return applyOps(DIExpression::getExtOps(FromTypeBitSize, ToTypeBitSize,
-                                            isa<SExtInst>(&I)));
+    auto ExtOps = DIExpression::getExtOps(FromTypeBitSize, ToTypeBitSize,
+                                          isa<SExtInst>(&I));
+    Ops.append(ExtOps.begin(), ExtOps.end());
+    return FromValue;
   }
 
-  SmallVector<uint64_t, 8> Ops;
-  if (auto *GEP = dyn_cast<GetElementPtrInst>(&I)) {
-    if (getSalvageOpsForGEP(GEP, DL, CurrentLocOps, Ops, AdditionalValues))
-      return doSalvage(Ops);
-  } else if (auto *BI = dyn_cast<BinaryOperator>(&I)) {
-    if (getSalvageOpsForBinOp(BI, CurrentLocOps, Ops, AdditionalValues))
-      return doSalvage(Ops);
+  if (auto *GEP = dyn_cast<GetElementPtrInst>(&I))
+    return getSalvageOpsForGEP(GEP, DL, CurrentLocOps, Ops, AdditionalValues);
+  else if (auto *BI = dyn_cast<BinaryOperator>(&I)) {
+    return getSalvageOpsForBinOp(BI, CurrentLocOps, Ops, AdditionalValues);
   }
   // *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;
 }
 
 /// A replacement for a dbg.value expression.