[Debuginfo][LSR] Add support for salvaging variadic dbg.value intrinsics [1/2] [NFC]

[chrisjackson]

Summary

First of two patches that will enable salvaging of dbg.value instrinsics that
have multiple locations ops before the Loop Strength Reduction pass.

The existing single-op SCEV-based salvaging can generate variadic dbg.value
intrinsics in order to salvage a dbg.value that has a single location op.
If a dbg.value has multiple location ops before LSR, and LSR optimises
away one or more of the location operands, then no salvaging will be attempted.

Salvaging can now be added, but first this patch cleans up consistency
in both the code and comments, and applies some refactoring to make application
of the new salvaging implementation more straightforward.

• Use SCEVDbgValueBuilder for both types of recovery expressions: IV-offset based and iteration count based.
• Combine the functions that write the final DIExpression.
• Move some static functions into member functions.

[chrisjackson]

Applied clang-format.

[Orlando]

Can this be labelled NFC or is there a functional change here that I've missed?

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6060

Should this be ...from a given value... rather than ...for a given value...?

6061

nit: I think these function names should all start with lower case.

6069

nit: the function names in this file appear to have a mix of capitalization style -- since it's already inconsistent it might be better to follow the standard (camelCase)? Not a strong opinion, feel free to ignore.

6138–6139

Please can you explain why this is necessary (on phab or in the comment or both)?

[chrisjackson]

Responded to review from @Orlando (Thanks!).

• Improved the comments with respect to writing shorter dbg.value. Its quite hard to explain concisely that DIArgList and {DW_OP_LLVM_arg, 0} aren't always necessary, but I tried.

• Renamed functions that had been moved inside SCEVDbgValueBuilder and were incorrectly cased.

• Yes, this patch is NFC, but I wasn't sure of the benefits / rules for labeling it as such. But I'm happy to do so.

[Orlando]

I don't think there are "rules" about tagging patches as NFC (just put NFC somewhere in the title). It's definitely helpful to be able to see at a glance though - two examples: if an NFC patch is causing changes in output there's a bug, and if a patch is NFC it probably doesn't need any tests.

Nearly ready to approve but I'm still stuck on one thing (sorry if I'm just missing something obvious!).

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6143

My previous inline comment seems to have sunk further down, so I'm making a new one here. Sorry for holding things up but I still don't really understand what these few lines are for - I tried looking in the pre-patched code but couldn't find any similar code. What is the intended condition here and why does a DW_OP_stack_value need to be added under that condition?

[chrisjackson]

In D120168#3354027, @Orlando wrote:

I don't think there are "rules" about tagging patches as NFC (just put NFC somewhere in the title). It's definitely helpful to be able to see at a glance though - two examples: if an NFC patch is causing changes in output there's a bug, and if a patch is NFC it probably doesn't need any tests.

Nearly ready to approve but I'm still stuck on one thing (sorry if I'm just missing something obvious!).

I can clarify the comment. It's my understanding that non-empty DIExpressions must have a DW_OP_stack_value terminator. The expressions generated by the SCEVDbgValueBuilder objects do not have these terminators (as the expressions may be inserted into other expressions). If the pre-LSR expression was empty, once the complete salvaging expression has been generated, then the terminator must be appended.

[chrisjackson]

Correction to failed function renaming.

[chrisjackson]

Attempt to make UpdateDbgValueInst() more clear when adding the DW_OP_stack_value terminator.

[Orlando]

I can clarify the comment. It's my understanding that non-empty DIExpressions must have a DW_OP_stack_value terminator. The expressions generated by the SCEVDbgValueBuilder objects do not have these terminators (as the expressions may be inserted into other expressions). If the pre-LSR expression was empty, once the complete salvaging expression has been generated, then the terminator must be appended.

My main confusion was in why the addition of stack_value was necessary in the patch but wasn't added before. I'd missed that you were previously using prependOpcodes with param stackValue=true, but are now adding the opcode yourself. And I agree that adding a stack_value is correct in this instance because you're generating implicit locations (though I think "non-empty DIExpressions must have a DW_OP_stack_value terminator" is not true in general?). In any case, sorry for the protracted review as a result of my misunderstanding!

prependOpcodes also accounts for inserting a stack_value opcode before fragment opcodes, which your implementation doesn't appear to handle. Would it be possible to rearrange the code to use prependOpcodes so that all cases are accounted for?

[chrisjackson]

In D120168#3360280, @Orlando wrote:

I can clarify the comment. It's my understanding that non-empty DIExpressions must have a DW_OP_stack_value terminator. The expressions generated by the SCEVDbgValueBuilder objects do not have these terminators (as the expressions may be inserted into other expressions). If the pre-LSR expression was empty, once the complete salvaging expression has been generated, then the terminator must be appended.

My main confusion was in why the addition of stack_value was necessary in the patch but wasn't added before. I'd missed that you were previously using prependOpcodes with param stackValue=true, but are now adding the opcode yourself. And I agree that adding a stack_value is correct in this instance because you're generating implicit locations (though I think "non-empty DIExpressions must have a DW_OP_stack_value terminator" is not true in general?). In any case, sorry for the protracted review as a result of my misunderstanding!

prependOpcodes also accounts for inserting a stack_value opcode before fragment opcodes, which your implementation doesn't appear to handle. Would it be possible to rearrange the code to use prependOpcodes so that all cases are accounted for?

I understand your query now.

EDIT: This was good advice and I've changed the code accordingly.

[chrisjackson]

Removed the naive code to check and add dwarf::DW_OP_stack_value in UpdateDbgValueInst() following @Orlando 's helpful advice and reverted to using DIExpression::prependOpcodes().

My previous reason for not following the advice didn't actually make sense for this first patch in the series.

[Orlando]

The change SGTM but I found a reproducer that causes clang to crash when this patch is applied:

$ cat reduce.cpp
#include <utility>
template <typename a> class Vec {
public:
  void push(a);
};
void d(int argc, char **argv) {
  Vec<std::pair<char *, unsigned>> e;
  for (int f = 0;;)
    for (; f < argc; ++f)
      e.push(std::make_pair(argv[f], f));
}

$ clang++  -O2 -g reduce.cpp

/home/och/dev/llvm/llvm-project/llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h:139: llvm::DbgValueLoc::DbgValueLoc(const llvm::DIExpression*, llvm::ArrayRef<llvm::DbgValueLocEntry>, bool): Assertion `Expr && Expr->isValid() && is_contained(Expr->getElements(), dwarf::DW_OP_stack_value)' failed.
...

[chrisjackson]

In D120168#3363344, @Orlando wrote:

The change SGTM but I found a reproducer that causes clang to crash when this patch is applied:

$ cat reduce.cpp
#include <utility>
template <typename a> class Vec {
public:
  void push(a);
};
void d(int argc, char **argv) {
  Vec<std::pair<char *, unsigned>> e;
  for (int f = 0;;)
    for (; f < argc; ++f)
      e.push(std::make_pair(argv[f], f));
}

$ clang++  -O2 -g reduce.cpp

/home/och/dev/llvm/llvm-project/llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h:139: llvm::DbgValueLoc::DbgValueLoc(const llvm::DIExpression*, llvm::ArrayRef<llvm::DbgValueLocEntry>, bool): Assertion `Expr && Expr->isValid() && is_contained(Expr->getElements(), dwarf::DW_OP_stack_value)' failed.
...

Oops, looks like when I split my patch into two I failed to keep some critical code in this patch. Sometimes LSR itself modifies the number of location ops in a dbg.value. If scev-based salvaging is to attempt recovery it must first restore the dbg.value to a single-op state by removing the DIArglist in the first argument. Without this restoration the salvage attempts will create invalid dbg.value, hence the crash you observed (thanks again!).

I will readd the code that removes DIArglists added by LSR.

[chrisjackson]

Reinstate the check for dbg.value that LSR has changed from single location-op to multi location-op. When LSR makes this change, the single location argument is changed to a DIArglist. This patch now replaces the DIArglist with a single 'undef'. This will prevent the generation of invalid dbg.value that were causing the crash as reported by @Orlando .

[chrisjackson]

Remove accidental double application of dbg.value expression restoration.

[Orlando]

In D120168#3363746, @chrisjackson wrote:

Reinstate the check for dbg.value that LSR has changed from single location-op to multi location-op. When LSR makes this change, the single location argument is changed to a DIArglist. This patch now replaces the DIArglist with a single 'undef'. This will prevent the generation of invalid dbg.value that were causing the crash as reported by @Orlando .

If that change makes the patch not-NFC, please could you add a test for this and mention it in the commit message? Lastly, please can you appease the clang-format bot?

[chrisjackson]

In D120168#3367442, @Orlando wrote:

In D120168#3363746, @chrisjackson wrote:

Reinstate the check for dbg.value that LSR has changed from single location-op to multi location-op. When LSR makes this change, the single location argument is changed to a DIArglist. This patch now replaces the DIArglist with a single 'undef'. This will prevent the generation of invalid dbg.value that were causing the crash as reported by @Orlando .

If that change makes the patch not-NFC, please could you add a test for this and mention it in the commit message? Lastly, please can you appease the clang-format bot?

Thinking about it, the codeblock in which this improvement exists is removed entirely in patch [2/2]. So I will revert to the old method of simply giving up on the salvage if the undef location is now nullptr (and thus the type is unavailable) . This will reinstate the NFC status of this patch.

The original purpose of the patch was to refactor and clean things up for the new implementation in patch [2/2], so i think leaving out this improvement is the most sensible course of action. I could generate a test but it doesn't seem worth it for code that would be removed in the near future.

[Orlando]

Makes sense to restore NFC-ness of the patch, LGTM with that change.

[chrisjackson]

Restore the the old behaviour for transforming DIArglist location arguments to single- location op:. When trying to determine the type of an undef Value that has become a nullptr, simply abandon the salvage attempt. This restores the NFC status of this patch.

In the second patch in this series, this code block is removed.

[chrisjackson]

Add check for empty arglist that was missing from check for nullptr arglist member

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

void LoopStrengthReduce::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addPreserved<IVUsersWrapperPass>();
   AU.addRequired<TargetTransformInfoWrapperPass>();
   AU.addPreserved<MemorySSAWrapperPass>();
 }
 
 namespace {
 struct SCEVDbgValueBuilder {
   SCEVDbgValueBuilder() = default;
-  SCEVDbgValueBuilder(const SCEVDbgValueBuilder &Base) {
+  SCEVDbgValueBuilder(const SCEVDbgValueBuilder &Base) { clone(Base); }
+
+  void clone(const SCEVDbgValueBuilder &Base) {
     Values = Base.Values;
     Expr = Base.Expr;
   }
 
   /// The DIExpression as we translate the SCEV.
   SmallVector<uint64_t, 6> Expr;
   /// The location ops of the DIExpression.
-  SmallVector<llvm::ValueAsMetadata *, 2> Values;
+  SmallVector<Value *, 2> Values;
 
   void pushOperator(uint64_t Op) { Expr.push_back(Op); }
   void pushUInt(uint64_t Operand) { Expr.push_back(Operand); }
 
   /// Add a DW_OP_LLVM_arg to the expression, followed by the index of the value
   /// in the set of values referenced by the expression.
   void pushValue(llvm::Value *V) {
     Expr.push_back(llvm::dwarf::DW_OP_LLVM_arg);
-    auto *It =
-        std::find(Values.begin(), Values.end(), llvm::ValueAsMetadata::get(V));
+    auto *It = std::find(Values.begin(), Values.end(), V);
     unsigned ArgIndex = 0;
     if (It != Values.end()) {
       ArgIndex = std::distance(Values.begin(), It);
     } else {
       ArgIndex = Values.size();
-      Values.push_back(llvm::ValueAsMetadata::get(V));
+      Values.push_back(V);
     }
     Expr.push_back(ArgIndex);
   }
 
   void pushValue(const SCEVUnknown *U) {
     llvm::Value *V = cast<SCEVUnknown>(U)->getValue();
     pushValue(V);
   }
if (const SCEVConstant *StartInt = dyn_cast<SCEVConstant>(S)) {
       return false;
 
     } else {
       return false;
     }
     return Success;
   }
 
-  void setFinalExpression(llvm::DbgValueInst &DI, const DIExpression *OldExpr) {
-    // Re-state assumption that this dbg.value is not variadic. Any remaining
-    // opcodes in its expression operate on a single value already on the
-    // expression stack. Prepend our operations, which will re-compute and
-    // place that value on the expression stack.
-    assert(!DI.hasArgList());
-    auto *NewExpr =
-        DIExpression::prependOpcodes(OldExpr, Expr, /*StackValue*/ true);
-    DI.setExpression(NewExpr);
-
-    auto ValArrayRef = llvm::ArrayRef<llvm::ValueAsMetadata *>(Values);
-    DI.setRawLocation(llvm::DIArgList::get(DI.getContext(), ValArrayRef));
-  }
-
-  /// If a DVI can be emitted without a DIArgList, omit DW_OP_llvm_arg and the
-  /// location op index 0.
-  void setShortFinalExpression(llvm::DbgValueInst &DI,
-                               const DIExpression *OldExpr) {
-    assert((Expr[0] == llvm::dwarf::DW_OP_LLVM_arg && Expr[1] == 0) &&
-           "Expected DW_OP_llvm_arg and 0.");
-    DI.replaceVariableLocationOp(
-        0u, llvm::MetadataAsValue::get(DI.getContext(), Values[0]));
-
-    // See setFinalExpression: prepend our opcodes on the start of any old
-    // expression opcodes.
-    assert(!DI.hasArgList());
-    llvm::SmallVector<uint64_t, 6> FinalExpr(llvm::drop_begin(Expr, 2));
-    auto *NewExpr =
-        DIExpression::prependOpcodes(OldExpr, FinalExpr, /*StackValue*/ true);
-    DI.setExpression(NewExpr);
-  }
-
-  /// Once the IV and variable SCEV translation is complete, write it to the
-  /// source DVI.
-  void applyExprToDbgValue(llvm::DbgValueInst &DI,
-                           const DIExpression *OldExpr) {
-    assert(!Expr.empty() && "Unexpected empty expression.");
-    // Emit a simpler form if only a single location is referenced.
-    if (Values.size() == 1 && Expr[0] == llvm::dwarf::DW_OP_LLVM_arg &&
-        Expr[1] == 0) {
-      setShortFinalExpression(DI, OldExpr);
-    } else {
-      setFinalExpression(DI, OldExpr);
-    }
-  }
-
   /// Return true if the combination of arithmetic operator and underlying
   /// SCEV constant value is an identity function.
   bool isIdentityFunction(uint64_t Op, const SCEV *S) {
     if (const SCEVConstant *C = dyn_cast<SCEVConstant>(S)) {
       if (C->getAPInt().getMinSignedBits() > 64)
         return false;
       int64_t I = C->getAPInt().getSExtValue();
       switch (Op) {
bool SCEVToValueExpr(const llvm::SCEVAddRecExpr &SAR, ScalarEvolution &SE) {
     if (!isIdentityFunction(llvm::dwarf::DW_OP_plus, Start)) {
       if (!pushSCEV(Start))
         return false;
       pushOperator(llvm::dwarf::DW_OP_plus);
     }
     return true;
   }
 
+  /// Create an expression that is an offset from a value (usually the IV).
+  void createOffsetExpr(int64_t Offset, Value *OffsetValue) {
+    pushValue(OffsetValue);
+    DIExpression::appendOffset(Expr, Offset);
+    LLVM_DEBUG(dbgs() << "scev-salvage: Generated IV offset expression. Offset: "  << std::to_string(Offset) << "\n");
+  }
+
+  /// Combine a translation of the SCEV and the IV to create an expression that
+  /// recovers a location's value.
+  void createIterCountExpr(const SCEV *S,
+                           const SCEVDbgValueBuilder &IterationCount,
+                           ScalarEvolution &SE) {
+    // SCEVs for SSA values are most frquently of the form
+    // {start,+,stride}, but sometimes they are ({start,+,stride} + %a + ..).
+    // This is because %a is a PHI node that is not the IV. However, these
+    // SCEVs have not been observed to result in debuginfo-lossy optimisations,
+    // so its not expected this point will be reached.
+    if (!isa<SCEVAddRecExpr>(S))
+      return;
+
+    LLVM_DEBUG(dbgs() << "scev-salvage: Value to salvage SCEV: " << *S << '\n');
+
+    const auto *Rec = cast<SCEVAddRecExpr>(S);
+    if (!Rec->isAffine())
+      return;
+
+    if (S->getExpressionSize() > MaxSCEVSalvageExpressionSize)
+      return;
+
+    // Initialise a new builder with the iteration count expression. In
+    // combination with the value's SCEV this enables recovery.
+    clone(IterationCount);
+    if (!SCEVToValueExpr(*Rec, SE))
+      return;
+  }
+
   /// Convert a SCEV of a value to a DIExpression that is pushed onto the
   /// builder's expression stack. The stack should already contain an
   /// expression for the iteration count, so that it can be multiplied by
   /// the stride and added to the start.
   /// Components of the expression are omitted if they are an identity function.
   bool SCEVToIterCountExpr(const llvm::SCEVAddRecExpr &SAR,
                            ScalarEvolution &SE) {
     assert(SAR.isAffine() && "Expected affine SCEV");
if (!isIdentityFunction(llvm::dwarf::DW_OP_div, Stride)) {
       if (!pushSCEV(Stride))
         return false;
       pushOperator(llvm::dwarf::DW_OP_div);
     }
     return true;
   }
 };
 
+/// Holds all the required data to salvage a dbg.value using the pre-LSR SCEVs
+/// and DIExpression.
 struct DVIRecoveryRec {
   DbgValueInst *DVI;
   DIExpression *Expr;
   Metadata *LocationOp;
   const llvm::SCEV *SCEV;
 };
 } // namespace
 
-static void RewriteDVIUsingIterCount(DVIRecoveryRec CachedDVI,
-                                     const SCEVDbgValueBuilder &IterationCount,
-                                     ScalarEvolution &SE) {
-  // LSR may add locations to previously single location-op DVIs which
-  // are currently not supported.
-  if (CachedDVI.DVI->getNumVariableLocationOps() != 1)
-    return;
+/// Write the new expression and new location ops for the dbg.value. Emit as
+/// short a expression as possible based on checks of the expression length and
+/// number of location ops.
+static void UpdateDbgValueInst(DVIRecoveryRec &DVIRec,
+                               SmallVectorImpl<Value *> &NewLocationOps,
+                               SmallVectorImpl<uint64_t> &NewExpr) {
+
+  // If there is only a single location op, the {DW_OP_LLVM_arg, 0} sequence
+  // can be omitted from the expression. Also, DIArglist() can be ommitted from
+  // the first argument of the dbg.value.
+  if (NewLocationOps.size() == 1 && NewExpr[0] == dwarf::DW_OP_LLVM_arg &&
+      NewExpr[1] == 0) {
+    DVIRec.DVI->replaceVariableLocationOp(0u, NewLocationOps[0]);
+    llvm::SmallVector<uint64_t, 6> ShortExpr(llvm::drop_begin(NewExpr, 2));
+    auto *FinalExpr = DIExpression::prependOpcodes(DVIRec.Expr, ShortExpr,
+                                                   /*StackValue*/ true);
+    DVIRec.DVI->setExpression(FinalExpr);
+  } else {
+    SmallVector<ValueAsMetadata *, 3> MetadataLocs;
+    for (Value *V : NewLocationOps)
+      MetadataLocs.push_back(ValueAsMetadata::get(V));
+    auto ValArrayRef = llvm::ArrayRef<llvm::ValueAsMetadata *>(MetadataLocs);
+    DVIRec.DVI->setRawLocation(
+        llvm::DIArgList::get(DVIRec.DVI->getContext(), ValArrayRef));
+    auto *FinalExpr =
+        DIExpression::prependOpcodes(DVIRec.Expr, NewExpr, /*StackValue*/ true);
+    DVIRec.DVI->setExpression(FinalExpr);
+  }
+}
 
-  // SCEVs for SSA values are most frquently of the form
-  // {start,+,stride}, but sometimes they are ({start,+,stride} + %a + ..).
-  // This is because %a is a PHI node that is not the IV. However, these
-  // SCEVs have not been observed to result in debuginfo-lossy optimisations,
-  // so its not expected this point will be reached.
-  if (!isa<SCEVAddRecExpr>(CachedDVI.SCEV))
-    return;
+static bool SalvageDVI(llvm::Loop *L, ScalarEvolution &SE,
+                       llvm::PHINode *LSRInductionVar, DVIRecoveryRec &DVIRec,
+                       const SCEV *SCEVInductionVar,
+                       SCEVDbgValueBuilder IterCountExpr) {
+  if (!DVIRec.DVI->isUndef())
+    return false;
 
-  LLVM_DEBUG(dbgs() << "scev-salvage: Value to salvage SCEV: "
-                    << *CachedDVI.SCEV << '\n');
+  // Restore the expression to pre-LSR state so that is consistent with the
+  // cached SCEVs and Values.
+  DVIRec.DVI->setExpression(DVIRec.Expr);
 
-  const auto *Rec = cast<SCEVAddRecExpr>(CachedDVI.SCEV);
-  if (!Rec->isAffine())
-    return;
+  // Some DVIs that were single location-op when cached are now multi-op,
+  // due to LSR optimisations. However, multi-op salvaging is not yet
+  // supported by SCEV salvaging. But, we can attempt a salvage by restoring
+  // the pre-LSR single-op expression. The expression applies to a single
+  // location op, so the DIAarglist must be replaced by the original location,
+  // which is now undef.
+  if (DVIRec.DVI->hasArgList()) {
+    // Cannot attempt to retrieve the type of the optimised away location
+    // as it may no longer be available, so just use Int64 because either the
+    // undef is overwritten by a successful salvage or remains undef.
+    DVIRec.DVI->setRawLocation(llvm::ValueAsMetadata::get(
+        UndefValue::get(llvm::Type::getInt64Ty(DVIRec.DVI->getContext()))));
+    DVIRec.DVI->setExpression(DVIRec.Expr);
+  }
 
-  if (CachedDVI.SCEV->getExpressionSize() > MaxSCEVSalvageExpressionSize)
-    return;
+  LLVM_DEBUG(dbgs() << "scev-salvage: attempt to salvage: " << *DVIRec.DVI
+                    << '\n');
 
-  // Initialise a new builder with the iteration count expression. In
-  // combination with the value's SCEV this enables recovery.
-  SCEVDbgValueBuilder RecoverValue(IterationCount);
-  if (!RecoverValue.SCEVToValueExpr(*Rec, SE))
-    return;
+  SCEVDbgValueBuilder SalvageExpr;
+
+  // Create an offset-based salvage expression if possible, as it requires
+  // less DWARF ops than an iteration count-based expression.
+  if (Optional<APInt> Offset =
+          SE.computeConstantDifference(DVIRec.SCEV, SCEVInductionVar)) {
+    if (Offset.getValue().getMinSignedBits() <= 64)
+      SalvageExpr.createOffsetExpr(Offset.getValue().getSExtValue(),
+                                   LSRInductionVar);
+  } else
+    SalvageExpr.createIterCountExpr(DVIRec.SCEV, IterCountExpr, SE);
 
-  LLVM_DEBUG(dbgs() << "scev-salvage: Updating: " << *CachedDVI.DVI << '\n');
-  RecoverValue.applyExprToDbgValue(*CachedDVI.DVI, CachedDVI.Expr);
-  LLVM_DEBUG(dbgs() << "scev-salvage: to: " << *CachedDVI.DVI << '\n');
-}
-
-static void RewriteDVIUsingOffset(DVIRecoveryRec &DVIRec, llvm::PHINode &IV,
-                                  int64_t Offset) {
-  assert(!DVIRec.DVI->hasArgList() && "Expected single location-op dbg.value.");
-  DbgValueInst *DVI = DVIRec.DVI;
-  SmallVector<uint64_t, 8> Ops;
-  DIExpression::appendOffset(Ops, Offset);
-  DIExpression *Expr = DIExpression::prependOpcodes(DVIRec.Expr, Ops, true);
-  LLVM_DEBUG(dbgs() << "scev-salvage: Updating: " << *DVIRec.DVI << '\n');
-  DVI->setExpression(Expr);
-  llvm::Value *ValIV = dyn_cast<llvm::Value>(&IV);
-  DVI->replaceVariableLocationOp(
-      0u, llvm::MetadataAsValue::get(DVI->getContext(),
-                                     llvm::ValueAsMetadata::get(ValIV)));
-  LLVM_DEBUG(dbgs() << "scev-salvage: updated with offset to IV: "
-                    << *DVIRec.DVI << '\n');
+  UpdateDbgValueInst(DVIRec, SalvageExpr.Values, SalvageExpr.Expr);
+
+  LLVM_DEBUG(dbgs() << "scev-salvage: Updated DVI: " << *DVIRec.DVI << "\n");
+  return true;
 }
 
+/// Obtain an expression for the iteration count, then attempt to salvage the
+/// dbg.value intrinsics.
 static void
 DbgRewriteSalvageableDVIs(llvm::Loop *L, ScalarEvolution &SE,
                           llvm::PHINode *LSRInductionVar,
                           SmallVector<DVIRecoveryRec, 2> &DVIToUpdate) {
   if (DVIToUpdate.empty())
     return;
 
   const llvm::SCEV *SCEVInductionVar = SE.getSCEV(LSRInductionVar);
   assert(SCEVInductionVar &&
          "Anticipated a SCEV for the post-LSR induction variable");
 
   if (const SCEVAddRecExpr *IVAddRec =
           dyn_cast<SCEVAddRecExpr>(SCEVInductionVar)) {
     if (!IVAddRec->isAffine())
       return;
 
+    // Prevent translation using excessive resources.
     if (IVAddRec->getExpressionSize() > MaxSCEVSalvageExpressionSize)
       return;
 
     // The iteration count is required to recover location values.
     SCEVDbgValueBuilder IterCountExpr;
     IterCountExpr.pushValue(LSRInductionVar);
     if (!IterCountExpr.SCEVToIterCountExpr(*IVAddRec, SE))
       return;
 
     LLVM_DEBUG(dbgs() << "scev-salvage: IV SCEV: " << *SCEVInductionVar
                       << '\n');
 
-    // Needn't salvage if the location op hasn't been undef'd by LSR.
     for (auto &DVIRec : DVIToUpdate) {
-      if (!DVIRec.DVI->isUndef())
-        continue;
-
-      // Some DVIs that were single location-op when cached are now multi-op,
-      // due to LSR optimisations. However, multi-op salvaging is not yet
-      // supported by SCEV salvaging. But, we can attempt a salvage by restoring
-      // the pre-LSR single-op expression.
-      if (DVIRec.DVI->hasArgList()) {
-        if (!DVIRec.DVI->getVariableLocationOp(0))
-          continue;
-        llvm::Type *Ty = DVIRec.DVI->getVariableLocationOp(0)->getType();
-        DVIRec.DVI->setRawLocation(
-            llvm::ValueAsMetadata::get(UndefValue::get(Ty)));
-        DVIRec.DVI->setExpression(DVIRec.Expr);
-      }
-
-      LLVM_DEBUG(dbgs() << "scev-salvage: value to recover SCEV: "
-                        << *DVIRec.SCEV << '\n');
-
-      // Create a simple expression if the IV and value to salvage SCEVs
-      // start values differ by only a constant value.
-      if (Optional<APInt> Offset =
-              SE.computeConstantDifference(DVIRec.SCEV, SCEVInductionVar)) {
-        if (Offset.getValue().getMinSignedBits() <= 64)
-          RewriteDVIUsingOffset(DVIRec, *LSRInductionVar,
-                                Offset.getValue().getSExtValue());
-      } else {
-        RewriteDVIUsingIterCount(DVIRec, IterCountExpr, SE);
-      }
+      SalvageDVI(L, SE, LSRInductionVar, DVIRec, SCEVInductionVar,
+                 IterCountExpr);
     }
   }
 }
 
 /// Identify and cache salvageable DVI locations and expressions along with the
 /// corresponding SCEV(s). Also ensure that the DVI is not deleted between
 /// cacheing and salvaging.
 static void