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

This change causes an assert / segmentation fault in LTO builds.

This reverts commit f2e4f3e.

Author: amykhuang

llvm/include/llvm/IR/DebugInfoMetadata.h

@@ -2594,16 +2594,6 @@ class DIExpression : public MDNode {
   // return it's sign information.
   llvm::Optional<SignedOrUnsignedConstant> isConstant() const;
 
-  /// Return the number of unique location operands referred to (via
-  /// DW_OP_LLVM_arg) in this expression; this is not necessarily the number of
-  /// instances of DW_OP_LLVM_arg within the expression.
-  /// For example, for the expression:
-  ///   (DW_OP_LLVM_arg 0, DW_OP_LLVM_arg 1, DW_OP_plus,
-  ///    DW_OP_LLVM_arg 0, DW_OP_mul)
-  /// This function would return 2, as there are two unique location operands
-  /// (0 and 1).
-  uint64_t getNumLocationOperands() const;
-
   using element_iterator = ArrayRef<uint64_t>::iterator;
 
   element_iterator elements_begin() const { return getElements().begin(); }
@@ -2751,10 +2741,6 @@ class DIExpression : public MDNode {
   /// return true with an offset of zero.
   bool extractIfOffset(int64_t &Offset) const;
 
-  /// Returns true iff this DIExpression contains at least one instance of
-  /// `DW_OP_LLVM_arg, n` for all n in [0, N).
-  bool hasAllLocationOps(unsigned N) const;
-
   /// Checks if the last 4 elements of the expression are DW_OP_constu <DWARF
   /// Address Space> DW_OP_swap DW_OP_xderef and extracts the <DWARF Address
   /// Space>.

llvm/include/llvm/IR/Instructions.h

@@ -1122,9 +1122,7 @@ class GetElementPtrInst : public Instruction {
   /// must be at least as wide as the IntPtr type for the address space of
   /// the base GEP pointer.
   bool accumulateConstantOffset(const DataLayout &DL, APInt &Offset) const;
-  bool collectOffset(const DataLayout &DL, unsigned BitWidth,
-                     SmallDenseMap<Value *, APInt, 8> &VariableOffsets,
-                     APInt &ConstantOffset) const;
+
   // Methods for support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const Instruction *I) {
     return (I->getOpcode() == Instruction::GetElementPtr);

llvm/include/llvm/IR/IntrinsicInst.h

@@ -204,11 +204,6 @@ class DbgVariableIntrinsic : public DbgInfoIntrinsic {
 
   void replaceVariableLocationOp(Value *OldValue, Value *NewValue);
   void replaceVariableLocationOp(unsigned OpIdx, Value *NewValue);
-  /// Adding a new location operand will always result in this intrinsic using
-  /// an ArgList, and must always be accompanied by a new expression that uses
-  /// the new operand.
-  void addVariableLocationOps(ArrayRef<Value *> NewValues,
-                              DIExpression *NewExpr);
 
   void setVariable(DILocalVariable *NewVar) {
     setArgOperand(1, MetadataAsValue::get(NewVar->getContext(), NewVar));

llvm/include/llvm/IR/Operator.h

@@ -576,12 +576,6 @@ class GEPOperator
       Type *SourceType, ArrayRef<const Value *> Index, const DataLayout &DL,
       APInt &Offset,
       function_ref<bool(Value &, APInt &)> ExternalAnalysis = nullptr);
-
-  /// Collect the offset of this GEP as a map of Values to their associated
-  /// APInt multipliers, as well as a total Constant Offset.
-  bool collectOffset(const DataLayout &DL, unsigned BitWidth,
-                     SmallDenseMap<Value *, APInt, 8> &VariableOffsets,
-                     APInt &ConstantOffset) const;
 };
 
 class PtrToIntOperator

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

@@ -314,8 +314,7 @@ void salvageDebugInfoForDbgValues(Instruction &I,
 /// 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);
+                                   bool StackVal, unsigned LocNo);
 
 /// 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

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

@@ -1238,10 +1238,6 @@ void SelectionDAGBuilder::resolveDanglingDebugInfo(const Value *V,
 }
 
 void SelectionDAGBuilder::salvageUnresolvedDbgValue(DanglingDebugInfo &DDI) {
-  // TODO: For the variadic implementation, instead of only checking the fail
-  // state of `handleDebugValue`, we need know specifically which values were
-  // invalid, so that we attempt to salvage only those values when processing
-  // a DIArgList.
   assert(!DDI.getDI()->hasArgList() &&
          "Not implemented for variadic dbg_values");
   Value *V = DDI.getDI()->getValue(0);
@@ -1265,21 +1261,16 @@ void SelectionDAGBuilder::salvageUnresolvedDbgValue(DanglingDebugInfo &DDI) {
   while (isa<Instruction>(V)) {
     Instruction &VAsInst = *cast<Instruction>(V);
     // Temporary "0", awaiting real implementation.
-    SmallVector<Value *, 4> AdditionalValues;
-    DIExpression *SalvagedExpr =
-        salvageDebugInfoImpl(VAsInst, Expr, StackValue, 0, AdditionalValues);
+    DIExpression *NewExpr = salvageDebugInfoImpl(VAsInst, Expr, StackValue, 0);
 
     // If we cannot salvage any further, and haven't yet found a suitable debug
     // expression, bail out.
-    // 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 (!NewExpr)
       break;
 
     // New value and expr now represent this debuginfo.
     V = VAsInst.getOperand(0);
-    Expr = SalvagedExpr;
+    Expr = NewExpr;
 
     // Some kind of simplification occurred: check whether the operand of the
     // salvaged debug expression can be encoded in this DAG.

llvm/lib/IR/DebugInfoMetadata.cpp

@@ -1244,17 +1244,6 @@ bool DIExpression::extractIfOffset(int64_t &Offset) const {
   return false;
 }
 
-bool DIExpression::hasAllLocationOps(unsigned N) const {
-  SmallDenseSet<uint64_t, 4> SeenOps;
-  for (auto ExprOp : expr_ops())
-    if (ExprOp.getOp() == dwarf::DW_OP_LLVM_arg)
-      SeenOps.insert(ExprOp.getArg(0));
-  for (uint64_t Idx = 0; Idx < N; ++Idx)
-    if (!is_contained(SeenOps, Idx))
-      return false;
-  return true;
-}
-
 const DIExpression *DIExpression::extractAddressClass(const DIExpression *Expr,
                                                       unsigned &AddrClass) {
   // FIXME: This seems fragile. Nothing that verifies that these elements
@@ -1469,16 +1458,6 @@ Optional<DIExpression *> DIExpression::createFragmentExpression(
   return DIExpression::get(Expr->getContext(), Ops);
 }
 
-uint64_t DIExpression::getNumLocationOperands() const {
-  uint64_t Result = 0;
-  for (auto ExprOp : expr_ops())
-    if (ExprOp.getOp() == dwarf::DW_OP_LLVM_arg)
-      Result = std::max(Result, ExprOp.getArg(0) + 1);
-  assert(hasAllLocationOps(Result) &&
-         "Expression is missing one or more location operands.");
-  return Result;
-}
-
 llvm::Optional<DIExpression::SignedOrUnsignedConstant>
 DIExpression::isConstant() const {
 

llvm/lib/IR/Instructions.cpp

@@ -1806,15 +1806,6 @@ bool GetElementPtrInst::accumulateConstantOffset(const DataLayout &DL,
   return cast<GEPOperator>(this)->accumulateConstantOffset(DL, Offset);
 }
 
-bool GetElementPtrInst::collectOffset(
-    const DataLayout &DL, unsigned BitWidth,
-    SmallDenseMap<Value *, APInt, 8> &VariableOffsets,
-    APInt &ConstantOffset) const {
-  // Delegate to the generic GEPOperator implementation.
-  return cast<GEPOperator>(this)->collectOffset(DL, BitWidth, VariableOffsets,
-                                                ConstantOffset);
-}
-
 //===----------------------------------------------------------------------===//
 //                           ExtractElementInst Implementation
 //===----------------------------------------------------------------------===//

llvm/lib/IR/IntrinsicInst.cpp

@@ -118,23 +118,6 @@ void DbgVariableIntrinsic::replaceVariableLocationOp(unsigned OpIdx,
       0, MetadataAsValue::get(getContext(), DIArgList::get(getContext(), MDs)));
 }
 
-void DbgVariableIntrinsic::addVariableLocationOps(ArrayRef<Value *> NewValues,
-                                                  DIExpression *NewExpr) {
-  assert(NewExpr->hasAllLocationOps(getNumVariableLocationOps() +
-                                    NewValues.size()) &&
-         "NewExpr for debug variable intrinsic does not reference every "
-         "location operand.");
-  assert(!is_contained(NewValues, nullptr) && "New values must be non-null");
-  setArgOperand(2, MetadataAsValue::get(getContext(), NewExpr));
-  SmallVector<ValueAsMetadata *, 4> MDs;
-  for (auto *VMD : location_ops())
-    MDs.push_back(getAsMetadata(VMD));
-  for (auto *VMD : NewValues)
-    MDs.push_back(getAsMetadata(VMD));
-  setArgOperand(
-      0, MetadataAsValue::get(getContext(), DIArgList::get(getContext(), MDs)));
-}
-
 Optional<uint64_t> DbgVariableIntrinsic::getFragmentSizeInBits() const {
   if (auto Fragment = getExpression()->getFragmentInfo())
     return Fragment->SizeInBits;

llvm/lib/IR/Operator.cpp

@@ -142,61 +142,4 @@ bool GEPOperator::accumulateConstantOffset(
   }
   return true;
 }
-
-bool GEPOperator::collectOffset(
-    const DataLayout &DL, unsigned BitWidth,
-    SmallDenseMap<Value *, APInt, 8> &VariableOffsets,
-    APInt &ConstantOffset) const {
-  assert(BitWidth == DL.getIndexSizeInBits(getPointerAddressSpace()) &&
-         "The offset bit width does not match DL specification.");
-
-  auto CollectConstantOffset = [&](APInt Index, uint64_t Size) {
-    Index = Index.sextOrTrunc(BitWidth);
-    APInt IndexedSize = APInt(BitWidth, Size);
-    ConstantOffset += Index * IndexedSize;
-  };
-
-  for (gep_type_iterator GTI = gep_type_begin(this), GTE = gep_type_end(this);
-       GTI != GTE; ++GTI) {
-    // Scalable vectors are multiplied by a runtime constant.
-    bool ScalableType = isa<ScalableVectorType>(GTI.getIndexedType());
-
-    Value *V = GTI.getOperand();
-    StructType *STy = GTI.getStructTypeOrNull();
-    // Handle ConstantInt if possible.
-    if (auto ConstOffset = dyn_cast<ConstantInt>(V)) {
-      if (ConstOffset->isZero())
-        continue;
-      // If the type is scalable and the constant is not zero (vscale * n * 0 =
-      // 0) bailout.
-      // TODO: If the runtime value is accessible at any point before DWARF
-      // emission, then we could potentially keep a forward reference to it
-      // in the debug value to be filled in later.
-      if (ScalableType)
-        return false;
-      // Handle a struct index, which adds its field offset to the pointer.
-      if (STy) {
-        unsigned ElementIdx = ConstOffset->getZExtValue();
-        const StructLayout *SL = DL.getStructLayout(STy);
-        // Element offset is in bytes.
-        CollectConstantOffset(APInt(BitWidth, SL->getElementOffset(ElementIdx)),
-                              1);
-        continue;
-      }
-      CollectConstantOffset(ConstOffset->getValue(),
-                            DL.getTypeAllocSize(GTI.getIndexedType()));
-      continue;
-    }
-
-    if (STy || ScalableType)
-      return false;
-    // Insert an initial offset of 0 for V iff none exists already, then
-    // increment the offset by IndexedSize.
-    VariableOffsets.try_emplace(V, BitWidth, 0);
-    APInt IndexedSize =
-        APInt(BitWidth, DL.getTypeAllocSize(GTI.getIndexedType()));
-    VariableOffsets[V] += IndexedSize;
-  }
-  return true;
-}
 } // namespace llvm

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

@@ -2101,15 +2101,10 @@ void coro::salvageDebugInfo(
     } 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);
-      // Debug declares cannot currently handle additional location
-      // operands.
-      if (!SalvagedExpr || !AdditionalValues.empty())
-        break;
-      Expr = SalvagedExpr;
+      Expr = llvm::salvageDebugInfoImpl(*GEPInst, Expr,
+                                        /*WithStackValue=*/false, 0);
+      if (!Expr)
+        return;
       Storage = GEPInst->getOperand(0);
     } else if (auto *BCInst = dyn_cast<llvm::BitCastInst>(Storage))
       Storage = BCInst->getOperand(0);