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

[chrisjackson]

Summary

Second of two patches to enable salvaging of dbg.value intrinsics that have
multiple location ops pre-LSR.

This second patch adds the core implementation. Much is shared with the
original, single-op SCEV-based salvaging. Each
optimised away location op is assigned a SCEVDbgValuebuilder that generates a
DIExpression and location ops that enable recovery of the location value. Most
of the complexity lies in creating the final location op DIArglist, because the
number of locations that will be optimised away is not known until after LSR,
and and the number of new locations that will be required to recover the
missing locations is not known until each SCEVDbgValueBuilder has generated a
recovery expression. The final DIArglist is organised as follows:

DIArglist(Induction Variable, [non optimised-out locations], [locations required for recovery expressions])

The final DIExpression is initialised empty. Its contents are formed by parsing
the original (pre-LSR) expression left-to-right, pushing each element onto the
new expression. When a DW_OP_LLVM_arg reference to an optimised-out location is
encountered, the SCEVDbgValueBuilder that recovers that location's value is
used to append both its recovery expression and values onto the final
expression and DIArglist (list contents shown above). If a reference to a
non-optimised out location is encountered, the argument index is adjusted using
a map of old to new location op indexes.

Using this method avoids any inserts and forms both the final location list and
expression with appends.

[djtodoro]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6434

is the Op really const?

llvm/test/Transforms/LoopStrengthReduce/debuginfo-scev-salvage-5.ll

82–83

do we need all of these?

120–123

I guess we don't need the TBAA for the test case.

[chrisjackson]

Removed unnecessary lit test contents as per review comments.
Fixed a comment typo in ScalarEvolution.h.

[chrisjackson]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6434

As I understand it. 'Op' itself is not modified, but its members are used to calculate a new Op for the new expression.

[StephenTozer]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6159

Could this method be implemented as a more general merge of locations, so that every location in LocationOps that already appears in DestLocations is reused, rather than just the IV?

[StephenTozer]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6210

This would be better as a SmallVector pre-allocated with nullptrs imo - besides the vector having faster lookup, it also would use much less space (DenseMap automatically allocates space for 64 key+value pairs, which is 4 times as many as a DIArgList should ever actually contain).

6377–6393

Same as the above comment for RecoveryExprs, this would probably be more efficient as a SmallVector with some placeholder value for "empty" (_UI64_MAX or something to that effect).

6389

Since VH is only used in this if-block, the above check could be folded in here as if (VH && !isa<UndefValue>(VH)) {

6398

This needs to be freed at some point, could be either in the DVIRecoveryRec destructor, or this could be a unique_ptr in DVIRecoveryRec instead (which is probably safer).

6402

[chrisjackson]

Thanks @StephenTozer , these are really helpful. I will be able to address them once patch [1/2] is done, which shouldn't be much longer, so hopefully I can address your comments at the end of the week.

[chrisjackson]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6159

I agree that would be the most efficient use of the location ops arguments, but at the time of writing I though this would add some additional complexity. However, it now seems not so difficult.

I had planned to to implement this in an additional patch, but I'm happy to add the functionality to this. But would it then be necessary to search the destination location vector to check if an append is necessary or location-reuse is possible?

6210

Agreed, I was concerned with the potential cost of DenseMap, though it is convenient.

6377–6393

Agreed, thanks.

6398

I thought I had taken care of this with the clear() methods. Perhaps I implemented them incorrectly. Either way, I agree unique_ptr would be nicer.

[chrisjackson]

A WIP update only, not ready for a re-review. Incorporated some of @StephenTozer 's suggestions e.g.

• replacing densemap with vector (though this still needs to be done for the location-op index)
• making use of unique_ptr, though this had some knock-on effects on other structures -Simplified a branch in UpdateDbgValueInst

There are still several changes to make, including restoring the pre-LSR cached dbg.value correctly. I have observed that some passes will create DIArgList with a single location. This is valid, but it means extra case-handling is necessary, as the number of location-ops does not indicate the absence or presence of DIArgList, the presence of which does effect the validity of the DIExpression.

[chrisjackson]

Mostly refactoring of the functions that write the final salvage dbg.value. Some small corrections after a Clang-3.4 build caught a couple of corner cases (it builds fine now).

[djtodoro]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6224

clang-format please

6275

no need for the variable here, it can be inlined into the call-site

[chrisjackson]

Apply @djtodoro 's change and clang-format.

[chrisjackson]

Change remaining Densemap (for updated location-op indexes) to a SmallVector.

@StephenTozer , with the exception of the location merge when appending expression and location vectors, I've completed all your suggestions. I'm keen to add that too but not in this patch, which I think is complex enough and I'm not sure how frequently locations would be duplicated. I would definitely add that in a subsequent patch but if you think that its really necessary now I can modify the append function.

[chrisjackson]

Update some variable names and a couple of comments to improve readability.

[StephenTozer]

Good patch - I've left quite a few comments, but most of them are minor nits and some are not requirements for this patch to be merged.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

5930

I suppose with the variable rename it may make sense to rename this function?

6091–6092

Nit, probably worth adding a comment for the new return value.

6159

Yeah, you'd need to check the DestLocations vector. I think it probably wouldn't be a significant performance cost to do this - the arrays are generally small (there's a relatively low hard limit on the number of ops) so it should be feasible to implement a solution in which you search DestLocations for each new LocationOp and then, if it is already present, you track that location's ArgIndex instead of adding a new one. Then you can look at that tracked index to find NewIndex. Example solution included below, though if it doesn't work and/or the problem proves more difficult than I've made it out to be then we could :

6169–6183

Example impl for the above suggestion.

6226–6230

There should probably be an assertion in this function that DW_OP_LLVM_arg appears either exactly once in the DIExpr as the first operator, or not at all. Actually, it is also possible that DW_OP_LLVM_arg, 0 could legally appear multiple times in the expression, which would be a little awkward to deal with. For example, if we salvaged the square of the IV, the expression might be DIExpr(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 0, DW_OP_mul).

I think the suitable answer for this patch would just be to drop such expressions for now, so that it's not a blocker. As a slight tangent though, there is a general solution possible; for an expression DIExpr([[A]], DW_OP_LLVM_arg, 0, [[B]]), it is possible to transform it to a non-variadic form like so: DIExpr(DW_OP_dup, [[A]], DW_OP_swap, [[B]]). Personally I'd be happy with this not being added in this patch though, it's a bit much imo.

6275–6277

This check could be simplified and made more correct by instead using DIExpr->isComplex(), since that also checks for DW_OP_LLVM_fragment and DW_OP_LLVM_tag_offset (which don't force stack_value). Also, as in the previous patch, it'd be better to use DIExpression's prepend function that accounts for fragments than directly appending the stack_value.

6303

Nit

6398

As above, this could use isComplex() instead.

6417

6499–6500

Doesn't need to be dealt with in this patch, just a suggestion that might be worth looking at later:

It would make sense to add an early continue here (and in the loop below) if LocOp is a constant, since we never need to salvage those ops anyway; though if DVI contains a non-SCEVable constant Op it's probably using non-SCEVable values anyway, so I don't think we'd lose many values tothise. The implementation would also touch a bunch of code around here, so is probably not worth the effort right now.

llvm/test/Transforms/LoopStrengthReduce/debuginfo-scev-salvage-5.ll

22

I think this is the fourth value now?

80–81

Can remove these I think.

[chrisjackson]

Respond to the very helpful review from @StephenTozer. Includes:

• SCEVDbgValueBuilder.appendToVectors() now merges location vectors, preventing duplicates
• Improved check for when a shorter DIExpression can be emitted
• Use isComplex() instead of the size of the expression vector

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6169–6183

Thanks!

6226–6230

I've added a lambda to iterate over the expression and check fo multiple DW_OP_LLVM_arg uses.

6275–6277

I don't think prepend() functions as desired here, as it will call prependOpCodes() with an empty Ops, which prevents the stack terminator being applied.

[StephenTozer]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6247

This does a good job of recognizing when the location use is complex, but it doesn't correctly handle the case where DW_OP_LLVM_arg, 0 appears later in the expression. The block for if (HasSimpleLocationUse) is correct, but the else still assumes that we just have a simple non-variadic expression. What we need is 3 different cases:

1. Single use of arg 0 at the front: (DW_OP_LLVM_arg, 0, ...) -> Drop the first two elements of the expression.
2. Non-variadic expression: (DW_OP_plus_uconst, 5, DW_OP_stack_value) -> Just set the unmodified expression.
3. Complex use of arg 0: (DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 0, ...) -> Drop the value altogether (or fixup with DW_OP_dup and DW_OP_swap, not recommended for this patch).

Right now this code handles the first two, and treats 3 the same as 2, which would result in an expression that does not use an arglist but contains DW_OP_LLVM_arg, which is currently an error.

6275–6277

My bad, that's correct - carry on. Just as long as SalvageExpression is guaranteed to not have DW_OP_stack_value already this should be fine.

[chrisjackson]

Attempt to correct the final overwrite of the dbg.value, based on the count of DW_OP_llvm_arg operands in the expression.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6247

Ok, I had failed to make my intentions clear with these functions. But the method wa messy anyway, I've tried to simplify things by adding the function numLLVMArgOps that determines the number of DW_OP_LLVM_ARG ops present in an expression. The count is then used to determine if a DIArglist should be used and if the expression can be shortened. I am still not completely happy with the implementation but i think it is an improvement.

[chrisjackson]

Correct diff update file.

[chrisjackson]

Removed old comments, clarified messages in some asserts. Added some comments to methods.

[StephenTozer]

Still a further suggestion on the most recent change - it's a minor point, but necessary I think to not produce invalid expressions. Other than that, LGTM.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6263

6269–6282

This is mostly there, but the if in the NumLLVMArgs == 1 block looks off to me, as it implies that the single DW_OP_LLVM_arg might not be the first element in NewExpr. Rather than an if, it should be an assert - I don't believe we ever produce an expression that uses DW_OP_LLVM_arg in the middle of an expression and not at the start. Alternatively, if we want to accept such expressions (which I think is quite reasonable), then they should become DBG_VALUE_LISTs rather than DBG_VALUEs. Also, if there is only one LLVM_arg in the expression then its value should definitely be 0 - anything else would be a malformed expression (we assert elsewhere that we never skip an arg index, i.e. we cannot have an expr with args [0, 1, 3] - it must be [0, 1, 2] or [0, 1, 2, 3]).

Suggested implementation included!

[chrisjackson]

Made corrections to UpdateDbgValueInst() using @StephenTozer 's help.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6269–6282

I like this change a lot. I find your implementation clearer (and correct unlike mine!). I was not certain of the rules for DW_OP_LLVM_arg placement and this also makes it very clear. Thanks

[StephenTozer]

Sorry for the delay on approving this, LGTM!

Also the last issue I brought up looks to have been present in the previous LSR salvage patch - this patch should now fix that.

[chrisjackson]

Note that this patch also fixes Issue #55097.

[kstoimenov]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6512–6522

Looks like this is being leaked. Please revert or fix your change.

Buildbot: https://lab.llvm.org/buildbot/#/builders/5/builds/22795

18095==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 304 byte(s) in 2 object(s) allocated from:

#0 0x561ec299112d in operator new(unsigned long) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cpp:95:3
#1 0x561ec99687f8 in DbgGatherSalvagableDVI /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6531:32
#2 0x561ec99687f8 in ReduceLoopStrength(llvm::Loop*, llvm::IVUsers&, llvm::ScalarEvolution&, llvm::DominatorTree&, llvm::LoopInfo&, llvm::TargetTransformInfo const&, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::MemorySSA*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6592:3
#3 0x561ec99faa88 in (anonymous namespace)::LoopStrengthReduce::runOnLoop(llvm::Loop*, llvm::LPPassManager&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6680:10
#4 0x561ec6c5298f in llvm::LPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/LoopPass.cpp:195:27
#5 0x561ec8627c77 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1430:27
#6 0x561ec86425c0 in llvm::FPPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1476:16
#7 0x561ec8629b16 in runOnModule /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1545:27
#8 0x561ec8629b16 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:535:44
#9 0x561ec29ecce5 in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/opt/opt.cpp:1022:12
#10 0x7efe4632309a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a) (BuildId: eb6a5dd378d22b1e695984462a799cd4c81cdc22)

[kstoimenov]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6512–6522

This is a wrong patch. Disregard.

[kstoimenov]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6512–6522

Actually I ended up reverting it after all. The bot went green: https://lab.llvm.org/buildbot/#/builders/5/builds/22822

[chrisjackson]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6512–6522

Thanks for reverting this, I was no longer monitoring when the bot detected the leak. I'l=m working on the fix now.

[chrisjackson]

• Add managed DVIRecoveryRecords to address memory leak

llvm/include/llvm/Analysis/ScalarEvolution.h

public:
   getStrengthenedNoWrapFlagsFromBinOp(const OverflowingBinaryOperator *OBO);
 
   /// Notify this ScalarEvolution that \p User directly uses SCEVs in \p Ops.
   void registerUser(const SCEV *User, ArrayRef<const SCEV *> Ops);
 
   /// Return true if the SCEV expression contains an undef value.
   bool containsUndefs(const SCEV *S) const;
 
+  /// Return true if the SCEV expression contains a Value that has been
+  /// optimised out and is now a nullptr.
+  bool containsErasedValue(const SCEV *S) const;
+
   /// Return a SCEV expression for the full generality of the specified
   /// expression.
   const SCEV *getSCEV(Value *V);
 
   const SCEV *getConstant(ConstantInt *V);
   const SCEV *getConstant(const APInt &Val);
   const SCEV *getConstant(Type *Ty, uint64_t V, bool isSigned = false);
   const SCEV *getLosslessPtrToIntExpr(const SCEV *Op, unsigned Depth = 0);

llvm/lib/Analysis/ScalarEvolution.cpp

 bool ScalarEvolution::containsUndefs(const SCEV *S) const {
   return SCEVExprContains(S, [](const SCEV *S) {
     if (const auto *SU = dyn_cast<SCEVUnknown>(S))
       return isa<UndefValue>(SU->getValue());
     return false;
   });
 }
 
+// Return true when S contains a value that is a nullptr.
+bool ScalarEvolution::containsErasedValue(const SCEV *S) const {
+  return SCEVExprContains(S, [](const SCEV *S) {
+    if (const auto *SU = dyn_cast<SCEVUnknown>(S))
+      return SU->getValue() == nullptr;
+    return false;
+  });
+}
+
 /// Return the size of an element read or written by Inst.
 const SCEV *ScalarEvolution::getElementSize(Instruction *Inst) {
   Type *Ty;
   if (StoreInst *Store = dyn_cast<StoreInst>(Inst))
     Ty = Store->getValueOperand()->getType();
   else if (LoadInst *Load = dyn_cast<LoadInst>(Inst))
     Ty = Load->getType();
   else

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

void LoopStrengthReduce::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addRequiredID(LoopSimplifyID);
   AU.addRequired<IVUsersWrapperPass>();
   AU.addPreserved<IVUsersWrapperPass>();
   AU.addRequired<TargetTransformInfoWrapperPass>();
   AU.addPreserved<MemorySSAWrapperPass>();
 }
 
 namespace {
+
+/// Enables more convenient iteration over a DWARF expression vector.
+static iterator_range<llvm::DIExpression::expr_op_iterator>
+ToDwarfOpIter(SmallVectorImpl<uint64_t> &Expr) {
+  llvm::DIExpression::expr_op_iterator Begin =
+      llvm::DIExpression::expr_op_iterator(Expr.begin());
+  llvm::DIExpression::expr_op_iterator End =
+      llvm::DIExpression::expr_op_iterator(Expr.end());
+  return {Begin, End};
+}
+
 struct SCEVDbgValueBuilder {
   SCEVDbgValueBuilder() = default;
   SCEVDbgValueBuilder(const SCEVDbgValueBuilder &Base) { clone(Base); }
 
   void clone(const SCEVDbgValueBuilder &Base) {
-    Values = Base.Values;
+    LocationOps = Base.LocationOps;
     Expr = Base.Expr;
   }
 
+  void clear() {
+    LocationOps.clear();
+    Expr.clear();
+  }
+
   /// The DIExpression as we translate the SCEV.
   SmallVector<uint64_t, 6> Expr;
   /// The location ops of the DIExpression.
-  SmallVector<Value *, 2> Values;
+  SmallVector<Value *, 2> LocationOps;
 
   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) {
+  void pushLocation(llvm::Value *V) {
     Expr.push_back(llvm::dwarf::DW_OP_LLVM_arg);
-    auto *It = std::find(Values.begin(), Values.end(), V);
+    auto *It = std::find(LocationOps.begin(), LocationOps.end(), V);
     unsigned ArgIndex = 0;
-    if (It != Values.end()) {
-      ArgIndex = std::distance(Values.begin(), It);
+    if (It != LocationOps.end()) {
+      ArgIndex = std::distance(LocationOps.begin(), It);
     } else {
-      ArgIndex = Values.size();
-      Values.push_back(V);
+      ArgIndex = LocationOps.size();
+      LocationOps.push_back(V);
     }
     Expr.push_back(ArgIndex);
   }
 
   void pushValue(const SCEVUnknown *U) {
     llvm::Value *V = cast<SCEVUnknown>(U)->getValue();
-    pushValue(V);
+    pushLocation(V);
   }
 
   bool pushConst(const SCEVConstant *C) {
     if (C->getAPInt().getMinSignedBits() > 64)
       return false;
     Expr.push_back(llvm::dwarf::DW_OP_consts);
     Expr.push_back(C->getAPInt().getSExtValue());
     return true;
   }
 
+  // Iterating the expression as DWARF ops is convenient when updating
+  // DWARF_OP_LLVM_args.
+  iterator_range<llvm::DIExpression::expr_op_iterator> expr_ops() {
+    return ToDwarfOpIter(Expr);
+  }
+
   /// Several SCEV types are sequences of the same arithmetic operator applied
   /// to constants and values that may be extended or truncated.
   bool pushArithmeticExpr(const llvm::SCEVCommutativeExpr *CommExpr,
                           uint64_t DwarfOp) {
     assert((isa<llvm::SCEVAddExpr>(CommExpr) || isa<SCEVMulExpr>(CommExpr)) &&
            "Expected arithmetic SCEV type");
     bool Success = true;
     unsigned EmitOperator = 0;
struct SCEVDbgValueBuilder {
   bool pushSCEV(const llvm::SCEV *S) {
     bool Success = true;
     if (const SCEVConstant *StartInt = dyn_cast<SCEVConstant>(S)) {
       Success &= pushConst(StartInt);
 
     } else if (const SCEVUnknown *U = dyn_cast<SCEVUnknown>(S)) {
       if (!U->getValue())
         return false;
-      pushValue(U->getValue());
+      pushLocation(U->getValue());
 
     } else if (const SCEVMulExpr *MulRec = dyn_cast<SCEVMulExpr>(S)) {
       Success &= pushArithmeticExpr(MulRec, llvm::dwarf::DW_OP_mul);
 
     } else if (const SCEVUDivExpr *UDiv = dyn_cast<SCEVUDivExpr>(S)) {
       Success &= pushSCEV(UDiv->getLHS());
       Success &= pushSCEV(UDiv->getRHS());
       pushOperator(llvm::dwarf::DW_OP_div);
if (!isIdentityFunction(llvm::dwarf::DW_OP_plus, 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);
+    pushLocation(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,
+  /// returns true if an expression was created.
+  bool 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;
+      return false;
 
-    LLVM_DEBUG(dbgs() << "scev-salvage: Value to salvage SCEV: " << *S << '\n');
+    LLVM_DEBUG(dbgs() << "scev-salvage: Location to salvage SCEV: " << *S
+                      << '\n');
 
     const auto *Rec = cast<SCEVAddRecExpr>(S);
     if (!Rec->isAffine())
-      return;
+      return false;
 
     if (S->getExpressionSize() > MaxSCEVSalvageExpressionSize)
-      return;
+      return false;
 
     // 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;
+      return false;
+
+    return true;
   }
 
   /// 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,
bool SCEVToIterCountExpr(const llvm::SCEVAddRecExpr &SAR,
     }
     if (!isIdentityFunction(llvm::dwarf::DW_OP_div, Stride)) {
       if (!pushSCEV(Stride))
         return false;
       pushOperator(llvm::dwarf::DW_OP_div);
     }
     return true;
   }
+
+  // Append the current expression and locations to a location list and an
+  // expression list. Modify the DW_OP_LLVM_arg indexes to account for
+  // the locations already present in the destination list.
+  // The IV is shared amongst all value recovery expressions, so it is left at
+  // Location index 0.
+  void appendToVectors(SmallVectorImpl<uint64_t> &DestExpr,
+                       SmallVectorImpl<Value *> &DestLocations,
+                       uint64_t LocationIndexOfIV = 0) {
+    assert(!DestLocations.empty() &&
+           "Expected the locations vector to contain the IV");
+    // The DWARF_OP_LLVM_arg arguments of the expression being appended must be
+    // modified to account for the locations already in the destination vector.
+    // All builders contain the IV as the first location op.
+    assert(!LocationOps.empty() &&
+           "Expected the location ops to contain the IV.");
+    // DestIndexMap[n] contains the index in DestLocations for the nth
+    // location in this SCEVDbgValueBuilder.
+    SmallVector<uint64_t, 2> DestIndexMap;
+    for (const auto &Op : LocationOps) {
+      auto It = find(DestLocations, Op);
+      if (It != DestLocations.end()) {
+        // Location already exists in DestLocations, reuse existing ArgIndex.
+        DestIndexMap.push_back(std::distance(DestLocations.begin(), It));
+        continue;
+      }
+      // Location is not in DestLocations, add it.
+      DestIndexMap.push_back(DestLocations.size());
+      DestLocations.push_back(Op);
+    }
+
+    for (const auto &Op : expr_ops()) {
+      if (Op.getOp() != dwarf::DW_OP_LLVM_arg) {
+        Op.appendToVector(DestExpr);
+        continue;
+      } 
+
+      DestExpr.push_back(dwarf::DW_OP_LLVM_arg);
+      // `DW_OP_LLVM_arg n` represents the nth LocationOp in this SCEV,
+      // DestIndexMap[n] contains its new index in DestLocations.
+      uint64_t NewIndex = DestIndexMap[Op.getArg(0)];
+      DestExpr.push_back(NewIndex);
+    }
+  }
 };
 
 /// Holds all the required data to salvage a dbg.value using the pre-LSR SCEVs
 /// and DIExpression.
 struct DVIRecoveryRec {
+  DVIRecoveryRec(DbgValueInst *DbgValue)
+      : DVI(DbgValue), Expr(DbgValue->getExpression()),
+        HadLocationArgList(false) {}
+
   DbgValueInst *DVI;
   DIExpression *Expr;
-  Metadata *LocationOp;
-  const llvm::SCEV *SCEV;
+  bool HadLocationArgList;
+  SmallVector<WeakVH, 2> LocationOps;
+  SmallVector<const llvm::SCEV *, 2> SCEVs;
+  SmallVector<std::unique_ptr<SCEVDbgValueBuilder>, 2> RecoveryExprs;
+
+  void clear() {
+    for (auto &RE : RecoveryExprs)
+      RE.reset();
+    RecoveryExprs.clear();
+  }
+
+  ~DVIRecoveryRec() { clear(); }
 };
 } // namespace
 
-/// 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.
+/// Returns the total number of DW_OP_llvm_arg operands in the expression.
+/// This is useful to determine if the expression requires the locations arg
+/// of the dbg.value to contain DIArglist.
+static unsigned numLLVMArgOps(SmallVectorImpl<uint64_t> &Expr) {
+  auto expr_ops = ToDwarfOpIter(Expr);
+  unsigned Count = 0;
+  for (auto Op : expr_ops)
+    if (Op.getOp() == dwarf::DW_OP_LLVM_arg)
+      Count++;
+  return Count;
+}
+
+/// Overwrites DVI with the location and Ops as the DIExpression. The expression
+/// must not require a DIArglist.This will create an invalid expression if Ops
+/// has any dwarf::DW_OP_llvm_arg operands.
+static void updateDVIWithLocation(DbgValueInst &DVI, Value *Location,
+                                  SmallVectorImpl<uint64_t> &Ops) {
+  assert(numLLVMArgOps(Ops) == 0 &&
+         "Expected expression that does not require DIArglist locations.");
+  DVI.setRawLocation(ValueAsMetadata::get(Location));
+  DVI.setExpression(DIExpression::get(DVI.getContext(), Ops));
+}
+
+/// Overwrite DVI with Locations placed into a DIArglist.
+static void updateDVIWithLocations(DbgValueInst &DVI,
+                                   SmallVectorImpl<Value *> &Locations,
+                                   SmallVectorImpl<uint64_t> &Ops) {
+  assert(numLLVMArgOps(Ops) != 0 &&
+         "Expected expression that requires DIArglist locations.");
+  SmallVector<ValueAsMetadata *, 3> MetadataLocs;
+  for (Value *V : Locations)
+    MetadataLocs.push_back(ValueAsMetadata::get(V));
+  auto ValArrayRef = llvm::ArrayRef<llvm::ValueAsMetadata *>(MetadataLocs);
+  DVI.setRawLocation(llvm::DIArgList::get(DVI.getContext(), ValArrayRef));
+  DVI.setExpression(DIExpression::get(DVI.getContext(), Ops));
+}
+
+/// Write the new expression and new location ops for the dbg.value. If possible
+/// omit DIArglist for a single-location expression.
 static void UpdateDbgValueInst(DVIRecoveryRec &DVIRec,
                                SmallVectorImpl<Value *> &NewLocationOps,
                                SmallVectorImpl<uint64_t> &NewExpr) {
+  unsigned NumLLVMArgs = numLLVMArgOps(NewExpr);
+  if (NumLLVMArgs == 0) {
+    // Location assumed to be on the stack.
+    updateDVIWithLocation(*DVIRec.DVI, NewLocationOps[0], NewExpr);
+  } else if (NumLLVMArgs > 1) {
+    // Multiple location references, DIArgList is stricly necessary.
+    updateDVIWithLocations(*DVIRec.DVI, NewLocationOps, NewExpr);
+  } else if (NumLLVMArgs == 1) {
+    // There is only a single location referenced by a single operand. If this
+    // is the first operand in the expression, it can be omitted along with
+    // DIArglist.
+    unsigned NumOpsToOmit = 0;
+    if (NewExpr[0] == dwarf::DW_OP_LLVM_arg && NewExpr[1] == 0)
+      NumOpsToOmit = 2;
+    llvm::SmallVector<uint64_t, 6> ShortenedOps(
+        llvm::drop_begin(NewExpr, NumOpsToOmit));
+    updateDVIWithLocation(*DVIRec.DVI, NewLocationOps[0], ShortenedOps);
+  }
+
+  // If the DIExpression was previously empty then add the terminator. Non-empty
+  // expressions have only had elements inserted into them and so the terminator
+  // should already be present e.g. stack_value or fragment.
+  DIExpression *SalvageExpr = DVIRec.DVI->getExpression();
+  if (!DVIRec.Expr->isComplex() && SalvageExpr->isComplex()) {
+    SalvageExpr = DIExpression::append(SalvageExpr, {dwarf::DW_OP_stack_value});
+    DVIRec.DVI->setExpression(SalvageExpr);
+  }
+}
+
+/// Cached location ops may be erased during LSR, in which case an undef is
+/// required when restoring from the cache. The type of that location is no
+/// longer available, so just use int8. The undef will be replaced by one or
+/// more locations later when a SCEVDbgValueBuilder selects alternative
+/// locations to use for the salvage.
+static Value *getValueOrUndef(WeakVH &VH, LLVMContext &C) {
+  return (VH) ? VH : UndefValue::get(llvm::Type::getInt8Ty(C));
+}
+
+/// Restore the DVI's pre-LSR arguments. Substitute undef for any erased values.
+static void restorePreTransformState(DVIRecoveryRec &DVIRec) {
+  LLVM_DEBUG(dbgs() << "scev-salvage: restore dbg.value to pre-LSR state\n"
+                    << "scev-salvage: post-LSR: " << *DVIRec.DVI << '\n');
+  assert(DVIRec.Expr && "Expected an expression");
+  DVIRec.DVI->setExpression(DVIRec.Expr);
 
-  // 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);
+  // Even a single location-op may be inside a DIArgList and referenced with
+  // DW_OP_LLVM_arg, which is valid only with a DIArgList.
+  if (!DVIRec.HadLocationArgList) {
+    assert(DVIRec.LocationOps.size() == 1 &&
+           "Unexpected number of location ops.");
+    // LSR's unsuccessful salvage attempt may have added DIArgList, which in this
+    // case was not present before, so force the location back to a single
+    // uncontained Value.
+    Value *CachedValue =
+        getValueOrUndef(DVIRec.LocationOps[0], DVIRec.DVI->getContext());
+    DVIRec.DVI->setRawLocation(ValueAsMetadata::get(CachedValue));
   } else {
     SmallVector<ValueAsMetadata *, 3> MetadataLocs;
-    for (Value *V : NewLocationOps)
-      MetadataLocs.push_back(ValueAsMetadata::get(V));
+    for (WeakVH VH : DVIRec.LocationOps) {
+      Value *CachedValue = getValueOrUndef(VH, DVIRec.DVI->getContext());
+      MetadataLocs.push_back(ValueAsMetadata::get(CachedValue));
+    }
     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);
   }
+  LLVM_DEBUG(dbgs() << "scev-salvage: pre-LSR: " << *DVIRec.DVI << '\n');
 }
 
 static bool SalvageDVI(llvm::Loop *L, ScalarEvolution &SE,
                        llvm::PHINode *LSRInductionVar, DVIRecoveryRec &DVIRec,
                        const SCEV *SCEVInductionVar,
                        SCEVDbgValueBuilder IterCountExpr) {
   if (!DVIRec.DVI->isUndef())
     return false;
 
-  // 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. 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()) {
-    if (DVIRec.DVI->getNumVariableLocationOps() == 0 ||
-        !DVIRec.DVI->getVariableLocationOp(0))
+  // LSR may have caused several changes to the dbg.value in the failed salvage
+  // attempt. So restore the DIExpression, the location ops and also the
+  // location ops format, which is always DIArglist for multiple ops, but only
+  // sometimes for a single op.
+  restorePreTransformState(DVIRec);
+
+  // LocationOpIndexMap[i] will store the post-LSR location index of
+  // the non-optimised out location at pre-LSR index i.
+  SmallVector<int64_t, 2> LocationOpIndexMap;
+  LocationOpIndexMap.assign(DVIRec.LocationOps.size(), -1);
+  SmallVector<Value *, 2> NewLocationOps;
+  NewLocationOps.push_back(LSRInductionVar);
+
+  for (unsigned i = 0; i < DVIRec.LocationOps.size(); i++) {
+    WeakVH VH = DVIRec.LocationOps[i];
+    // Place the locations not optimised out in the list first, avoiding
+    // inserts later. The map is used to update the DIExpression's
+    // DW_OP_LLVM_arg arguments as the expression is updated.
+    if (VH && !isa<UndefValue>(VH)) {
+      NewLocationOps.push_back(VH);
+      LocationOpIndexMap[i] = NewLocationOps.size() - 1;
+      LLVM_DEBUG(dbgs() << "scev-salvage: Location index " << i
+                        << " now at index " << LocationOpIndexMap[i] << "\n");
+      continue;
+    }
+
+    // It's possible that a value referred to in the SCEV may have been
+    // optimised out by LSR.
+    if (SE.containsErasedValue(DVIRec.SCEVs[i]) ||
+        SE.containsUndefs(DVIRec.SCEVs[i])) {
+      LLVM_DEBUG(dbgs() << "scev-salvage: SCEV for location at index: " << i
+                        << " refers to a location that is now undef or erased. "
+                           "Salvage abandoned.\n");
       return false;
-    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: attempt to salvage: " << *DVIRec.DVI
-                    << '\n');
+    LLVM_DEBUG(dbgs() << "scev-salvage: salvaging location at index " << i
+                      << " with SCEV: " << *DVIRec.SCEVs[i] << "\n");
 
-  SCEVDbgValueBuilder SalvageExpr;
+    DVIRec.RecoveryExprs[i] = std::make_unique<SCEVDbgValueBuilder>();
+    SCEVDbgValueBuilder *SalvageExpr = DVIRec.RecoveryExprs[i].get();
 
     // 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)) {
+            SE.computeConstantDifference(DVIRec.SCEVs[i], SCEVInductionVar)) {
       if (Offset.getValue().getMinSignedBits() <= 64)
-      SalvageExpr.createOffsetExpr(Offset.getValue().getSExtValue(),
+        SalvageExpr->createOffsetExpr(Offset.getValue().getSExtValue(),
                                       LSRInductionVar);
-  } else
-    SalvageExpr.createIterCountExpr(DVIRec.SCEV, IterCountExpr, SE);
+    } else if (!SalvageExpr->createIterCountExpr(DVIRec.SCEVs[i], IterCountExpr,
+                                                 SE))
+      return false;
+  }
 
-  UpdateDbgValueInst(DVIRec, SalvageExpr.Values, SalvageExpr.Expr);
+  // Merge the DbgValueBuilder generated expressions and the original
+  // DIExpression, place the result into an new vector.
+  SmallVector<uint64_t, 3> NewExpr;
+  if (DVIRec.Expr->getNumElements() == 0) {
+    assert(DVIRec.RecoveryExprs.size() == 1 &&
+           "Expected only a single recovery expression for an empty "
+           "DIExpression.");
+    assert(DVIRec.RecoveryExprs[0] &&
+           "Expected a SCEVDbgSalvageBuilder for location 0");
+    SCEVDbgValueBuilder *B = DVIRec.RecoveryExprs[0].get();
+    B->appendToVectors(NewExpr, NewLocationOps);
+  }
+  for (const auto &Op : DVIRec.Expr->expr_ops()) {
+    // Most Ops needn't be updated.
+    if (Op.getOp() != dwarf::DW_OP_LLVM_arg) {
+      Op.appendToVector(NewExpr);
+      continue;
+    }
 
+    uint64_t LocationArgIndex = Op.getArg(0);
+    SCEVDbgValueBuilder *DbgBuilder =
+        DVIRec.RecoveryExprs[LocationArgIndex].get();
+    // The location doesn't have s SCEVDbgValueBuilder, so LSR did not
+    // optimise it away. So just translate the argument to the updated
+    // location index.
+    if (!DbgBuilder) {
+      NewExpr.push_back(dwarf::DW_OP_LLVM_arg);
+      assert(LocationOpIndexMap[Op.getArg(0)] != -1 &&
+             "Expected a positive index for the location-op position.");
+      NewExpr.push_back(LocationOpIndexMap[Op.getArg(0)]);
+      continue;
+    }
+    // The location has a recovery expression.
+    DbgBuilder->appendToVectors(NewExpr, NewLocationOps);
+  }
+
+  UpdateDbgValueInst(DVIRec, NewLocationOps, NewExpr);
   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) {
+                          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);
+    IterCountExpr.pushLocation(LSRInductionVar);
     if (!IterCountExpr.SCEVToIterCountExpr(*IVAddRec, SE))
       return;
 
     LLVM_DEBUG(dbgs() << "scev-salvage: IV SCEV: " << *SCEVInductionVar
                       << '\n');
 
     for (auto &DVIRec : DVIToUpdate) {
-      SalvageDVI(L, SE, LSRInductionVar, DVIRec, SCEVInductionVar,
+      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
 DbgGatherSalvagableDVI(Loop *L, ScalarEvolution &SE,
-                       SmallVector<DVIRecoveryRec, 2> &SalvageableDVISCEVs,
+                       SmallVector<DVIRecoveryRec *, 2> &SalvageableDVISCEVs,
                        SmallSet<AssertingVH<DbgValueInst>, 2> &DVIHandles) {
   for (auto &B : L->getBlocks()) {
     for (auto &I : *B) {
       auto DVI = dyn_cast<DbgValueInst>(&I);
       if (!DVI)
         continue;
-
+      // Ensure that if any location op is undef that the dbg.vlue is not
+      // cached.
       if (DVI->isUndef())
         continue;
 
-      if (DVI->hasArgList())
-        continue;
-
-      if (!DVI->getVariableLocationOp(0) ||
-          !SE.isSCEVable(DVI->getVariableLocationOp(0)->getType()))
-        continue;
+      // Check that the location op SCEVs are suitable for translation to
+      // DIExpression.
+      const auto &HasTranslatableLocationOps =
+          [&](const DbgValueInst *DVI) -> bool {
+        for (const auto LocOp : DVI->location_ops()) {
+          if (!LocOp)
+            return false;
 
-      // SCEVUnknown wraps an llvm::Value, it does not have a start and stride.
-      // Therefore no translation to DIExpression is performed.
-      const SCEV *S = SE.getSCEV(DVI->getVariableLocationOp(0));
-      if (isa<SCEVUnknown>(S))
-        continue;
+          if (!SE.isSCEVable(LocOp->getType()))
+            return false;
 
-      // Avoid wasting resources generating an expression containing undef.
+          const SCEV *S = SE.getSCEV(LocOp);
           if (SE.containsUndefs(S))
+            return false;
+        }
+        return true;
+      };
+
+      if (!HasTranslatableLocationOps(DVI))
         continue;
 
-      SalvageableDVISCEVs.push_back(
-          {DVI, DVI->getExpression(), DVI->getRawLocation(),
-           SE.getSCEV(DVI->getVariableLocationOp(0))});
+      DVIRecoveryRec *NewRec = new DVIRecoveryRec(DVI);
+      // Each location Op may need a SCEVDbgValueBuilder in order to recover it.
+      // Pre-allocating a vector will enable quick lookups of the builder later
+      // during the salvage.
+      NewRec->RecoveryExprs.resize(DVI->getNumVariableLocationOps());
+      for (const auto LocOp : DVI->location_ops()) {
+        NewRec->SCEVs.push_back(SE.getSCEV(LocOp));
+        NewRec->LocationOps.push_back(LocOp);
+        NewRec->HadLocationArgList = DVI->hasArgList();
+      }
+      SalvageableDVISCEVs.push_back(NewRec);
       DVIHandles.insert(DVI);
     }
   }
 }
 
 /// Ideally pick the PHI IV inserted by ScalarEvolutionExpander. As a fallback
 /// any PHi from the loop header is usable, but may have less chance of
 /// surviving subsequent transforms.

 static bool ReduceLoopStrength(Loop *L, IVUsers &IU, ScalarEvolution &SE,
                                DominatorTree &DT, LoopInfo &LI,
                                const TargetTransformInfo &TTI,
                                AssumptionCache &AC, TargetLibraryInfo &TLI,
                                MemorySSA *MSSA) {
 
   // Debug preservation - before we start removing anything identify which DVI
   // meet the salvageable criteria and store their DIExpression and SCEVs.
-  SmallVector<DVIRecoveryRec, 2> SalvageableDVI;
+  SmallVector<DVIRecoveryRec *, 2> SalvageableDVIRecords;
   SmallSet<AssertingVH<DbgValueInst>, 2> DVIHandles;
-  DbgGatherSalvagableDVI(L, SE, SalvageableDVI, DVIHandles);
+  DbgGatherSalvagableDVI(L, SE, SalvageableDVIRecords, DVIHandles);
 
   bool Changed = false;
   std::unique_ptr<MemorySSAUpdater> MSSAU;
   if (MSSA)
     MSSAU = std::make_unique<MemorySSAUpdater>(MSSA);
 
   // Run the main LSR transformation.
   const LSRInstance &Reducer =
#endif
     if (numFolded) {
       Changed = true;
       RecursivelyDeleteTriviallyDeadInstructionsPermissive(DeadInsts, &TLI,
                                                            MSSAU.get());
       DeleteDeadPHIs(L->getHeader(), &TLI, MSSAU.get());
     }
   }
 
-  if (SalvageableDVI.empty())
+  if (SalvageableDVIRecords.empty())
     return Changed;
 
   // Obtain relevant IVs and attempt to rewrite the salvageable DVIs with
   // expressions composed using the derived iteration count.
   // TODO: Allow for multiple IV references for nested AddRecSCEVs
   for (auto &L : LI) {
     if (llvm::PHINode *IV = GetInductionVariable(*L, SE, Reducer))
-      DbgRewriteSalvageableDVIs(L, SE, IV, SalvageableDVI);
+      DbgRewriteSalvageableDVIs(L, SE, IV, SalvageableDVIRecords);
     else {
       LLVM_DEBUG(dbgs() << "scev-salvage: SCEV salvaging not possible. An IV "
                            "could not be identified.\n");
     }
   }
 
+  for (auto &Rec : SalvageableDVIRecords)
+    Rec->clear();
+  SalvageableDVIRecords.clear();
   DVIHandles.clear();
   return Changed;
 }
 
 bool LoopStrengthReduce::runOnLoop(Loop *L, LPPassManager & /*LPM*/) {
   if (skipLoop(L))
     return false;
 

llvm/test/Transforms/LoopStrengthReduce/debuginfo-scev-salvage-5.ll

+; RUN: opt -S -loop-reduce %s | FileCheck %s
+
+;; Ensure that SCEV-based salvaging in Loop Strength Reduction can salvage
+;; variadic dbg.value intrinsics. Generated from the following C++:
+
+;; clang -S -emit-llvm -Xclang -disable-llvm-passes -g lsr-variadic.cpp -o
+;; Then running 'opt -O2' up until LSR.
+;; void mul_to_addition(unsigned k, unsigned l, unsigned m, unsigned size, unsigned *data) {
+;;     unsigned i = 0;
+;; #pragma clang loop vectorize(disable)
+;;     while (i < size) {
+;;         unsigned comp = (4 * i) + k;
+;;         unsigned comp2 = comp * l;
+;;         unsigned comp3 = comp2 << m;
+;;         data[i] = comp;
+;;         i++;
+;;     }
+;; }
+;; This produces variadic dbg.value intrinsics with location op DIArglists 
+;; of length two and three.
+;; A fourth dbg.value was added artificially by copying a generated dbg.value
+;; and the modifying the position of the optimised-out value in the location
+;; list.
+
+; CHECK: call void @llvm.dbg.value(metadata !DIArgList(i64 %lsr.iv, i32 %k), metadata ![[comp:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_consts, 4, DW_OP_div, DW_OP_consts, 4, DW_OP_mul, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_stack_value))
+; CHECK: call void @llvm.dbg.value(metadata !DIArgList(i64 %lsr.iv, i32 %l, i32 %k), metadata ![[comp2:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_consts, 4, DW_OP_div, DW_OP_consts, 4, DW_OP_mul, DW_OP_LLVM_arg, 2, DW_OP_plus, DW_OP_LLVM_arg, 1, DW_OP_mul, DW_OP_stack_value))
+; CHECK: call void @llvm.dbg.value(metadata !DIArgList(i64 %lsr.iv, i32 %m, i32 %l, i32 %k), metadata ![[comp3:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_consts, 4, DW_OP_div, DW_OP_consts, 4, DW_OP_mul, DW_OP_LLVM_arg, 3, DW_OP_plus, DW_OP_LLVM_arg, 2, DW_OP_mul, DW_OP_LLVM_arg, 1, DW_OP_shl, DW_OP_stack_value))
+; CHECK: call void @llvm.dbg.value(metadata !DIArgList(i64 %lsr.iv, i32 %m, i32 %l, i32 %k), metadata ![[comp3:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_consts, 4, DW_OP_div, DW_OP_consts, 4, DW_OP_mul, DW_OP_LLVM_arg, 3, DW_OP_plus, DW_OP_LLVM_arg, 2, DW_OP_mul, DW_OP_LLVM_arg, 1, DW_OP_shl, DW_OP_stack_value))
+; CHECK: ![[comp]] = !DILocalVariable(name: "comp"
+; CHECK: ![[comp2]] = !DILocalVariable(name: "comp2"
+; CHECK: ![[comp3]] = !DILocalVariable(name: "comp3"
+
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define dso_local void @_Z15mul_to_additionjjjjPj(i32 %k, i32 %l, i32 %m, i32 %size, i32* nocapture %data) local_unnamed_addr #0 !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %k, metadata !14, metadata !DIExpression()), !dbg !24
+  call void @llvm.dbg.value(metadata i32 %l, metadata !15, metadata !DIExpression()), !dbg !24
+  call void @llvm.dbg.value(metadata i32 %m, metadata !16, metadata !DIExpression()), !dbg !24
+  call void @llvm.dbg.value(metadata i32 %size, metadata !17, metadata !DIExpression()), !dbg !24
+  call void @llvm.dbg.value(metadata i32* %data, metadata !18, metadata !DIExpression()), !dbg !24
+  call void @llvm.dbg.value(metadata i32 0, metadata !19, metadata !DIExpression()), !dbg !24
+  %cmp9.not = icmp eq i32 %size, 0, !dbg !25
+  br i1 %cmp9.not, label %while.end, label %while.body.preheader, !dbg !26
+
+while.body.preheader:                             ; preds = %entry
+  %wide.trip.count = zext i32 %size to i64, !dbg !25
+  br label %while.body, !dbg !26
+
+while.body:                                       ; preds = %while.body, %while.body.preheader
+  %indvars.iv = phi i64 [ 0, %while.body.preheader ], [ %indvars.iv.next, %while.body ]
+  call void @llvm.dbg.value(metadata i64 %indvars.iv, metadata !19, metadata !DIExpression()), !dbg !24
+  %0 = trunc i64 %indvars.iv to i32, !dbg !27
+  %mul = shl i32 %0, 2, !dbg !27
+  %add = add i32 %mul, %k, !dbg !28
+  call void @llvm.dbg.value(metadata i32 %add, metadata !20, metadata !DIExpression()), !dbg !29
+  call void @llvm.dbg.value(metadata !DIArgList(i32 %add, i32 %l), metadata !22, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_mul, DW_OP_stack_value)), !dbg !29
+  call void @llvm.dbg.value(metadata !DIArgList(i32 %add, i32 %m, i32 %l), metadata !23, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 2, DW_OP_mul, DW_OP_LLVM_arg, 1, DW_OP_shl, DW_OP_stack_value)), !dbg !29
+  call void @llvm.dbg.value(metadata !DIArgList(i32 %m, i32 %add, i32 %l), metadata !23, metadata !DIExpression(DW_OP_LLVM_arg, 1, DW_OP_LLVM_arg, 2, DW_OP_mul, DW_OP_LLVM_arg, 0, DW_OP_shl, DW_OP_stack_value)), !dbg !29
+  %arrayidx = getelementptr inbounds i32, i32* %data, i64 %indvars.iv, !dbg !30
+  store i32 %add, i32* %arrayidx, align 4, !dbg !31
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1, !dbg !32
+  call void @llvm.dbg.value(metadata i64 %indvars.iv.next, metadata !19, metadata !DIExpression()), !dbg !24
+  %exitcond = icmp ne i64 %indvars.iv.next, %wide.trip.count, !dbg !25
+  br i1 %exitcond, label %while.body, label %while.end.loopexit, !dbg !26, !llvm.loop !33
+
+while.end.loopexit:                               ; preds = %while.body
+  br label %while.end, !dbg !37
+
+while.end:                                        ; preds = %while.end.loopexit, %entry
+  ret void, !dbg !37
+}
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+attributes #0 = { "target-cpu"="x86-64" }
+
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 14.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "variadic2.cpp", directory: "/test")
+!2 = !{i32 7, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, !"uwtable", i32 1}
+!6 = !{!"clang version 14.0.0"}
+!7 = distinct !DISubprogram(name: "mul_to_addition", linkageName: "_Z15mul_to_additionjjjjPj", scope: !8, file: !8, line: 1, type: !9, scopeLine: 1, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
+!8 = !DIFile(filename: "./variadic2.cpp", directory: "/test")
+!9 = !DISubroutineType(types: !10)
+!10 = !{null, !11, !11, !11, !11, !12}
+!11 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64)
+!13 = !{!14, !15, !16, !17, !18, !19, !20, !22, !23}
+!14 = !DILocalVariable(name: "k", arg: 1, scope: !7, file: !8, line: 1, type: !11)
+!15 = !DILocalVariable(name: "l", arg: 2, scope: !7, file: !8, line: 1, type: !11)
+!16 = !DILocalVariable(name: "m", arg: 3, scope: !7, file: !8, line: 1, type: !11)
+!17 = !DILocalVariable(name: "size", arg: 4, scope: !7, file: !8, line: 1, type: !11)
+!18 = !DILocalVariable(name: "data", arg: 5, scope: !7, file: !8, line: 1, type: !12)
+!19 = !DILocalVariable(name: "i", scope: !7, file: !8, line: 2, type: !11)
+!20 = !DILocalVariable(name: "comp", scope: !21, file: !8, line: 5, type: !11)
+!21 = distinct !DILexicalBlock(scope: !7, file: !8, line: 4, column: 23)
+!22 = !DILocalVariable(name: "comp2", scope: !21, file: !8, line: 6, type: !11)
+!23 = !DILocalVariable(name: "comp3", scope: !21, file: !8, line: 7, type: !11)
+!24 = !DILocation(line: 0, scope: !7)
+!25 = !DILocation(line: 4, column: 15, scope: !7)
+!26 = !DILocation(line: 4, column: 6, scope: !7)
+!27 = !DILocation(line: 5, column: 29, scope: !21)
+!28 = !DILocation(line: 5, column: 34, scope: !21)
+!29 = !DILocation(line: 0, scope: !21)
+!30 = !DILocation(line: 8, column: 10, scope: !21)
+!31 = !DILocation(line: 8, column: 18, scope: !21)
+!32 = !DILocation(line: 9, column: 11, scope: !21)
+!33 = distinct !{!33, !26, !34, !35, !36}
+!34 = !DILocation(line: 10, column: 6, scope: !7)
+!35 = !{!"llvm.loop.mustprogress"}
+!36 = !{!"llvm.loop.vectorize.width", i32 1}
+!37 = !DILocation(line: 11, column: 2, scope: !7)