[llvm] 3bfddc2 - Reapply "[DebugInfo] Handle multiple variable location operands in IR"

Stephen Tozer via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 17 09:46:49 PDT 2021


Author: Stephen Tozer
Date: 2021-03-17T16:45:25Z
New Revision: 3bfddc25931d44da9b26c092f4e15634712b1459

URL: https://github.com/llvm/llvm-project/commit/3bfddc25931d44da9b26c092f4e15634712b1459
DIFF: https://github.com/llvm/llvm-project/commit/3bfddc25931d44da9b26c092f4e15634712b1459.diff

LOG: Reapply "[DebugInfo] Handle multiple variable location operands in IR"

Fixed section of code that iterated through a SmallDenseMap and added
instructions in each iteration, causing non-deterministic code; replaced
SmallDenseMap with MapVector to prevent non-determinism.

This reverts commit 01ac6d1587e8613ba4278786e8341f8b492ac941.

Added: 
    

Modified: 
    llvm/include/llvm/IR/IntrinsicInst.h
    llvm/include/llvm/Transforms/Utils/Local.h
    llvm/lib/CodeGen/CodeGenPrepare.cpp
    llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
    llvm/lib/IR/IntrinsicInst.cpp
    llvm/lib/IR/User.cpp
    llvm/lib/Target/AArch64/AArch64StackTagging.cpp
    llvm/lib/Transforms/Coroutines/CoroFrame.cpp
    llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
    llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
    llvm/lib/Transforms/Scalar/ADCE.cpp
    llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
    llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
    llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
    llvm/lib/Transforms/Utils/CodeExtractor.cpp
    llvm/lib/Transforms/Utils/Debugify.cpp
    llvm/lib/Transforms/Utils/Local.cpp
    llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
    llvm/unittests/IR/DebugInfoTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/IR/IntrinsicInst.h b/llvm/include/llvm/IR/IntrinsicInst.h
index a138e4bea8c0..4a2a747dd4bb 100644
--- a/llvm/include/llvm/IR/IntrinsicInst.h
+++ b/llvm/include/llvm/IR/IntrinsicInst.h
@@ -203,6 +203,7 @@ class DbgVariableIntrinsic : public DbgInfoIntrinsic {
   Value *getVariableLocationOp(unsigned OpIdx) const;
 
   void replaceVariableLocationOp(Value *OldValue, Value *NewValue);
+  void replaceVariableLocationOp(unsigned OpIdx, Value *NewValue);
 
   void setVariable(DILocalVariable *NewVar) {
     setArgOperand(1, MetadataAsValue::get(NewVar->getContext(), NewVar));

diff  --git a/llvm/include/llvm/Transforms/Utils/Local.h b/llvm/include/llvm/Transforms/Utils/Local.h
index dfcf289a30ec..f7efeeb56fd3 100644
--- a/llvm/include/llvm/Transforms/Utils/Local.h
+++ b/llvm/include/llvm/Transforms/Utils/Local.h
@@ -311,9 +311,10 @@ void salvageDebugInfoForDbgValues(Instruction &I,
 /// Given an instruction \p I and DIExpression \p DIExpr operating on it, write
 /// the effects of \p I into the returned DIExpression, or return nullptr if
 /// it cannot be salvaged. \p StackVal: whether DW_OP_stack_value should be
-/// appended to the expression.
+/// 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);
+                                   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

diff  --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 407c176d7b85..8e24c86dfcf2 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -2873,11 +2873,16 @@ class TypePromotionTransaction {
     /// Keep track of the debug users.
     SmallVector<DbgValueInst *, 1> DbgValues;
 
+    /// Keep track of the new value so that we can undo it by replacing
+    /// instances of the new value with the original value.
+    Value *New;
+
     using use_iterator = SmallVectorImpl<InstructionAndIdx>::iterator;
 
   public:
     /// Replace all the use of \p Inst by \p New.
-    UsesReplacer(Instruction *Inst, Value *New) : TypePromotionAction(Inst) {
+    UsesReplacer(Instruction *Inst, Value *New)
+        : TypePromotionAction(Inst), New(New) {
       LLVM_DEBUG(dbgs() << "Do: UsersReplacer: " << *Inst << " with " << *New
                         << "\n");
       // Record the original uses.
@@ -2903,7 +2908,7 @@ class TypePromotionTransaction {
       // the original debug uses must also be reinstated to maintain the
       // correctness and utility of debug value instructions.
       for (auto *DVI : DbgValues)
-        DVI->replaceVariableLocationOp(DVI->getVariableLocationOp(0), Inst);
+        DVI->replaceVariableLocationOp(New, Inst);
     }
   };
 
@@ -7903,18 +7908,21 @@ bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
   DbgValueInst &DVI = *cast<DbgValueInst>(I);
 
   // Does this dbg.value refer to a sunk address calculation?
-  Value *Location = DVI.getVariableLocationOp(0);
-  WeakTrackingVH SunkAddrVH = SunkAddrs[Location];
-  Value *SunkAddr = SunkAddrVH.pointsToAliveValue() ? SunkAddrVH : nullptr;
-  if (SunkAddr) {
-    // Point dbg.value at locally computed address, which should give the best
-    // opportunity to be accurately lowered. This update may change the type of
-    // pointer being referred to; however this makes no 
diff erence to debugging
-    // information, and we can't generate bitcasts that may affect codegen.
-    DVI.replaceVariableLocationOp(Location, SunkAddr);
-    return true;
-  }
-  return false;
+  bool AnyChange = false;
+  for (Value *Location : DVI.getValues()) {
+    WeakTrackingVH SunkAddrVH = SunkAddrs[Location];
+    Value *SunkAddr = SunkAddrVH.pointsToAliveValue() ? SunkAddrVH : nullptr;
+    if (SunkAddr) {
+      // Point dbg.value at locally computed address, which should give the best
+      // opportunity to be accurately lowered. This update may change the type
+      // of pointer being referred to; however this makes no 
diff erence to
+      // debugging information, and we can't generate bitcasts that may affect
+      // codegen.
+      DVI.replaceVariableLocationOp(Location, SunkAddr);
+      AnyChange = true;
+    }
+  }
+  return AnyChange;
 }
 
 // A llvm.dbg.value may be using a value before its definition, due to
@@ -7933,30 +7941,51 @@ bool CodeGenPrepare::placeDbgValues(Function &F) {
       if (!DVI)
         continue;
 
-      Instruction *VI = dyn_cast_or_null<Instruction>(DVI->getValue());
+      SmallVector<Instruction *, 4> VIs;
+      for (Value *V : DVI->getValues())
+        if (Instruction *VI = dyn_cast_or_null<Instruction>(V))
+          VIs.push_back(VI);
+
+      // This DVI may depend on multiple instructions, complicating any
+      // potential sink. This block takes the defensive approach, opting to
+      // "undef" the DVI if it has more than one instruction and any of them do
+      // not dominate DVI.
+      for (Instruction *VI : VIs) {
+        if (VI->isTerminator())
+          continue;
 
-      if (!VI || VI->isTerminator())
-        continue;
+        // If VI is a phi in a block with an EHPad terminator, we can't insert
+        // after it.
+        if (isa<PHINode>(VI) && VI->getParent()->getTerminator()->isEHPad())
+          continue;
 
-      // If VI is a phi in a block with an EHPad terminator, we can't insert
-      // after it.
-      if (isa<PHINode>(VI) && VI->getParent()->getTerminator()->isEHPad())
-        continue;
+        // If the defining instruction dominates the dbg.value, we do not need
+        // to move the dbg.value.
+        if (DT.dominates(VI, DVI))
+          continue;
 
-      // If the defining instruction dominates the dbg.value, we do not need
-      // to move the dbg.value.
-      if (DT.dominates(VI, DVI))
-        continue;
+        // If we depend on multiple instructions and any of them doesn't
+        // dominate this DVI, we probably can't salvage it: moving it to
+        // after any of the instructions could cause us to lose the others.
+        if (VIs.size() > 1) {
+          LLVM_DEBUG(
+              dbgs()
+              << "Unable to find valid location for Debug Value, undefing:\n"
+              << *DVI);
+          DVI->setUndef();
+          break;
+        }
 
-      LLVM_DEBUG(dbgs() << "Moving Debug Value before :\n"
-                        << *DVI << ' ' << *VI);
-      DVI->removeFromParent();
-      if (isa<PHINode>(VI))
-        DVI->insertBefore(&*VI->getParent()->getFirstInsertionPt());
-      else
-        DVI->insertAfter(VI);
-      MadeChange = true;
-      ++NumDbgValueMoved;
+        LLVM_DEBUG(dbgs() << "Moving Debug Value before :\n"
+                          << *DVI << ' ' << *VI);
+        DVI->removeFromParent();
+        if (isa<PHINode>(VI))
+          DVI->insertBefore(&*VI->getParent()->getFirstInsertionPt());
+        else
+          DVI->insertAfter(VI);
+        MadeChange = true;
+        ++NumDbgValueMoved;
+      }
     }
   }
   return MadeChange;

diff  --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index b1f61c688ebc..ffd4778a4a42 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -1260,7 +1260,8 @@ void SelectionDAGBuilder::salvageUnresolvedDbgValue(DanglingDebugInfo &DDI) {
   // variable. FIXME: Further work could recover those too.
   while (isa<Instruction>(V)) {
     Instruction &VAsInst = *cast<Instruction>(V);
-    DIExpression *NewExpr = salvageDebugInfoImpl(VAsInst, Expr, StackValue);
+    // Temporary "0", awaiting real implementation.
+    DIExpression *NewExpr = salvageDebugInfoImpl(VAsInst, Expr, StackValue, 0);
 
     // If we cannot salvage any further, and haven't yet found a suitable debug
     // expression, bail out.
@@ -6053,7 +6054,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
     DILocalVariable *Variable = DI.getVariable();
     DIExpression *Expression = DI.getExpression();
     dropDanglingDebugInfo(Variable, Expression);
-    SmallVector<Value *> Values(DI.getValues());
+    SmallVector<Value *, 4> Values(DI.getValues());
     if (Values.empty())
       return;
 

diff  --git a/llvm/lib/IR/IntrinsicInst.cpp b/llvm/lib/IR/IntrinsicInst.cpp
index 3d3f734ba5e0..55bc314f9ab3 100644
--- a/llvm/lib/IR/IntrinsicInst.cpp
+++ b/llvm/lib/IR/IntrinsicInst.cpp
@@ -98,6 +98,24 @@ void DbgVariableIntrinsic::replaceVariableLocationOp(Value *OldValue,
   setArgOperand(
       0, MetadataAsValue::get(getContext(), DIArgList::get(getContext(), MDs)));
 }
+void DbgVariableIntrinsic::replaceVariableLocationOp(unsigned OpIdx,
+                                                     Value *NewValue) {
+  assert(OpIdx < getNumVariableLocationOps() && "Invalid Operand Index");
+  if (!hasArgList()) {
+    Value *NewOperand = isa<MetadataAsValue>(NewValue)
+                            ? NewValue
+                            : MetadataAsValue::get(
+                                  getContext(), ValueAsMetadata::get(NewValue));
+    return setArgOperand(0, NewOperand);
+  }
+  SmallVector<ValueAsMetadata *, 4> MDs;
+  ValueAsMetadata *NewOperand = getAsMetadata(NewValue);
+  for (unsigned Idx = 0; Idx < getNumVariableLocationOps(); ++Idx)
+    MDs.push_back(Idx == OpIdx ? NewOperand
+                               : getAsMetadata(getVariableLocationOp(Idx)));
+  setArgOperand(
+      0, MetadataAsValue::get(getContext(), DIArgList::get(getContext(), MDs)));
+}
 
 Optional<uint64_t> DbgVariableIntrinsic::getFragmentSizeInBits() const {
   if (auto Fragment = getExpression()->getFragmentInfo())

diff  --git a/llvm/lib/IR/User.cpp b/llvm/lib/IR/User.cpp
index 9105c6fbd230..f4143163ab13 100644
--- a/llvm/lib/IR/User.cpp
+++ b/llvm/lib/IR/User.cpp
@@ -31,6 +31,10 @@ void User::replaceUsesOfWith(Value *From, Value *To) {
       // most importantly, removing "this" from the use list of "From".
       setOperand(i, To);
     }
+  if (auto DVI = dyn_cast_or_null<DbgVariableIntrinsic>(this)) {
+    if (is_contained(DVI->location_ops(), From))
+      DVI->replaceVariableLocationOp(From, To);
+  }
 }
 
 //===----------------------------------------------------------------------===//

diff  --git a/llvm/lib/Target/AArch64/AArch64StackTagging.cpp b/llvm/lib/Target/AArch64/AArch64StackTagging.cpp
index 793db06f79ad..e3938d9fe6fe 100644
--- a/llvm/lib/Target/AArch64/AArch64StackTagging.cpp
+++ b/llvm/lib/Target/AArch64/AArch64StackTagging.cpp
@@ -563,10 +563,9 @@ bool AArch64StackTagging::runOnFunction(Function &Fn) {
       }
 
       if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(I)) {
-        if (auto *AI =
-                dyn_cast_or_null<AllocaInst>(DVI->getVariableLocationOp(0))) {
-          Allocas[AI].DbgVariableIntrinsics.push_back(DVI);
-        }
+        for (Value *V : DVI->location_ops())
+          if (auto *AI = dyn_cast_or_null<AllocaInst>(V))
+            Allocas[AI].DbgVariableIntrinsics.push_back(DVI);
         continue;
       }
 

diff  --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index accd3a6ce16a..a6c6c4adb87f 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -2173,7 +2173,7 @@ void coro::salvageDebugInfo(
       Storage = StInst->getOperand(0);
     } else if (auto *GEPInst = dyn_cast<GetElementPtrInst>(Storage)) {
       Expr = llvm::salvageDebugInfoImpl(*GEPInst, Expr,
-                                        /*WithStackValue=*/false);
+                                        /*WithStackValue=*/false, 0);
       if (!Expr)
         return;
       Storage = GEPInst->getOperand(0);

diff  --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index d0c1cdb3188e..c8fb8aebe53a 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3586,15 +3586,6 @@ static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
   llvm::sort(DbgUsersToSink,
              [](auto *A, auto *B) { return B->comesBefore(A); });
 
-  // Update the arguments of a dbg.declare instruction, so that it
-  // does not point into a sunk instruction.
-  auto updateDbgDeclare = [](DbgVariableIntrinsic *DII) {
-    if (!isa<DbgDeclareInst>(DII))
-      return false;
-
-    return true;
-  };
-
   SmallVector<DbgVariableIntrinsic *, 2> DIIClones;
   SmallSet<DebugVariable, 4> SunkVariables;
   for (auto User : DbgUsersToSink) {
@@ -3602,7 +3593,7 @@ static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
     // one per variable fragment. It should be left in the original place
     // because the sunk instruction is not an alloca (otherwise we could not be
     // here).
-    if (updateDbgDeclare(User))
+    if (isa<DbgDeclareInst>(User))
       continue;
 
     DebugVariable DbgUserVariable =
@@ -3613,6 +3604,8 @@ static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
       continue;
 
     DIIClones.emplace_back(cast<DbgVariableIntrinsic>(User->clone()));
+    if (isa<DbgDeclareInst>(User) && isa<CastInst>(I))
+      DIIClones.back()->replaceVariableLocationOp(I, I->getOperand(0));
     LLVM_DEBUG(dbgs() << "CLONE: " << *DIIClones.back() << '\n');
   }
 

diff  --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
index e02076c74420..27715ff86ff2 100644
--- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
@@ -1218,10 +1218,10 @@ bool HWAddressSanitizer::sanitizeFunction(Function &F) {
           isa<CleanupReturnInst>(Inst))
         RetVec.push_back(&Inst);
 
-      if (auto *DDI = dyn_cast<DbgVariableIntrinsic>(&Inst))
-        if (auto *Alloca =
-                dyn_cast_or_null<AllocaInst>(DDI->getVariableLocationOp(0)))
-          AllocaDbgMap[Alloca].push_back(DDI);
+      if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst))
+        for (Value *V : DVI->location_ops())
+          if (auto *Alloca = dyn_cast_or_null<AllocaInst>(V))
+            AllocaDbgMap[Alloca].push_back(DVI);
 
       if (InstrumentLandingPads && isa<LandingPadInst>(Inst))
         LandingPadVec.push_back(&Inst);
@@ -1297,13 +1297,18 @@ bool HWAddressSanitizer::sanitizeFunction(Function &F) {
   }
 
   if (!AllocaToPaddedAllocaMap.empty()) {
-    for (auto &BB : F)
-      for (auto &Inst : BB)
-        if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst))
-          if (auto *AI =
-                  dyn_cast_or_null<AllocaInst>(DVI->getVariableLocationOp(0)))
-            if (auto *NewAI = AllocaToPaddedAllocaMap.lookup(AI))
-              DVI->replaceVariableLocationOp(AI, NewAI);
+    for (auto &BB : F) {
+      for (auto &Inst : BB) {
+        if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst)) {
+          for (Value *V : DVI->location_ops()) {
+            if (auto *AI = dyn_cast_or_null<AllocaInst>(V)) {
+              if (auto *NewAI = AllocaToPaddedAllocaMap.lookup(AI))
+                DVI->replaceVariableLocationOp(V, NewAI);
+            }
+          }
+        }
+      }
+    }
     for (auto &P : AllocaToPaddedAllocaMap)
       P.first->eraseFromParent();
   }

diff  --git a/llvm/lib/Transforms/Scalar/ADCE.cpp b/llvm/lib/Transforms/Scalar/ADCE.cpp
index c8fb9d106e32..6a31e71d80d0 100644
--- a/llvm/lib/Transforms/Scalar/ADCE.cpp
+++ b/llvm/lib/Transforms/Scalar/ADCE.cpp
@@ -521,10 +521,14 @@ bool AggressiveDeadCodeElimination::removeDeadInstructions() {
         // If intrinsic is pointing at a live SSA value, there may be an
         // earlier optimization bug: if we know the location of the variable,
         // why isn't the scope of the location alive?
-        if (Value *V = DII->getVariableLocationOp(0))
-          if (Instruction *II = dyn_cast<Instruction>(V))
-            if (isLive(II))
+        for (Value *V : DII->location_ops()) {
+          if (Instruction *II = dyn_cast<Instruction>(V)) {
+            if (isLive(II)) {
               dbgs() << "Dropping debug info for " << *DII << "\n";
+              break;
+            }
+          }
+        }
       }
     }
   });

diff  --git a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
index c71bb199b1ca..ae1ed681d998 100644
--- a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
@@ -5829,57 +5829,71 @@ void LoopStrengthReduce::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addPreserved<MemorySSAWrapperPass>();
 }
 
-using EqualValues = SmallVector<std::tuple<WeakVH, int64_t, DIExpression *>, 4>;
-using EqualValuesMap = DenseMap<DbgValueInst *, EqualValues>;
+using EqualValues = SmallVector<std::tuple<WeakVH, int64_t>, 4>;
+using EqualValuesMap =
+    DenseMap<std::pair<DbgValueInst *, unsigned>, EqualValues>;
+using ExpressionMap = DenseMap<DbgValueInst *, DIExpression *>;
 
 static void DbgGatherEqualValues(Loop *L, ScalarEvolution &SE,
-                                 EqualValuesMap &DbgValueToEqualSet) {
+                                 EqualValuesMap &DbgValueToEqualSet,
+                                 ExpressionMap &DbgValueToExpression) {
   for (auto &B : L->getBlocks()) {
     for (auto &I : *B) {
       auto DVI = dyn_cast<DbgValueInst>(&I);
       if (!DVI)
         continue;
-      auto V = DVI->getVariableLocationOp(0);
-      if (!V || !SE.isSCEVable(V->getType()))
-        continue;
-      auto DbgValueSCEV = SE.getSCEV(V);
-      EqualValues EqSet;
-      for (PHINode &Phi : L->getHeader()->phis()) {
-        if (V->getType() != Phi.getType())
-          continue;
-        if (!SE.isSCEVable(Phi.getType()))
+      for (unsigned Idx = 0; Idx < DVI->getNumVariableLocationOps(); ++Idx) {
+        // TODO: We can duplicate results if the same arg appears more than
+        // once.
+        Value *V = DVI->getVariableLocationOp(Idx);
+        if (!V || !SE.isSCEVable(V->getType()))
           continue;
-        auto PhiSCEV = SE.getSCEV(&Phi);
-        Optional<APInt> Offset =
-                SE.computeConstantDifference(DbgValueSCEV, PhiSCEV);
-        if (Offset && Offset->getMinSignedBits() <= 64)
-          EqSet.emplace_back(std::make_tuple(
-              &Phi, Offset.getValue().getSExtValue(), DVI->getExpression()));
+        auto DbgValueSCEV = SE.getSCEV(V);
+        EqualValues EqSet;
+        for (PHINode &Phi : L->getHeader()->phis()) {
+          if (V->getType() != Phi.getType())
+            continue;
+          if (!SE.isSCEVable(Phi.getType()))
+            continue;
+          auto PhiSCEV = SE.getSCEV(&Phi);
+          Optional<APInt> Offset =
+              SE.computeConstantDifference(DbgValueSCEV, PhiSCEV);
+          if (Offset && Offset->getMinSignedBits() <= 64)
+            EqSet.emplace_back(
+                std::make_tuple(&Phi, Offset.getValue().getSExtValue()));
+        }
+        DbgValueToEqualSet[{DVI, Idx}] = std::move(EqSet);
+        DbgValueToExpression[DVI] = DVI->getExpression();
       }
-      DbgValueToEqualSet[DVI] = std::move(EqSet);
     }
   }
 }
 
-static void DbgApplyEqualValues(EqualValuesMap &DbgValueToEqualSet) {
+static void DbgApplyEqualValues(EqualValuesMap &DbgValueToEqualSet,
+                                ExpressionMap &DbgValueToExpression) {
   for (auto A : DbgValueToEqualSet) {
-    auto DVI = A.first;
+    auto DVI = A.first.first;
+    auto Idx = A.first.second;
     // Only update those that are now undef.
-    if (!isa_and_nonnull<UndefValue>(DVI->getVariableLocationOp(0)))
+    if (!isa_and_nonnull<UndefValue>(DVI->getVariableLocationOp(Idx)))
       continue;
     for (auto EV : A.second) {
-      auto V = std::get<WeakVH>(EV);
-      if (!V)
+      auto EVHandle = std::get<WeakVH>(EV);
+      if (!EVHandle)
         continue;
-      auto DbgDIExpr = std::get<DIExpression *>(EV);
+      // The dbg.value may have had its value changed by LSR; refresh it from
+      // the map, but continue to update the mapped expression as it may be
+      // updated multiple times in this function.
+      auto DbgDIExpr = DbgValueToExpression[DVI];
       auto Offset = std::get<int64_t>(EV);
-      DVI->replaceVariableLocationOp(DVI->getVariableLocationOp(0), V);
+      DVI->replaceVariableLocationOp(Idx, EVHandle);
       if (Offset) {
         SmallVector<uint64_t, 8> Ops;
         DIExpression::appendOffset(Ops, Offset);
-        DbgDIExpr = DIExpression::prependOpcodes(DbgDIExpr, Ops, true);
+        DbgDIExpr = DIExpression::appendOpsToArg(DbgDIExpr, Ops, Idx, true);
       }
       DVI->setExpression(DbgDIExpr);
+      DbgValueToExpression[DVI] = DbgDIExpr;
       break;
     }
   }
@@ -5903,7 +5917,8 @@ static bool ReduceLoopStrength(Loop *L, IVUsers &IU, ScalarEvolution &SE,
   // Debug preservation - before we start removing anything create equivalence
   // sets for the llvm.dbg.value intrinsics.
   EqualValuesMap DbgValueToEqualSet;
-  DbgGatherEqualValues(L, SE, DbgValueToEqualSet);
+  ExpressionMap DbgValueToExpression;
+  DbgGatherEqualValues(L, SE, DbgValueToEqualSet, DbgValueToExpression);
 
   // Remove any extra phis created by processing inner loops.
   Changed |= DeleteDeadPHIs(L->getHeader(), &TLI, MSSAU.get());
@@ -5923,7 +5938,7 @@ static bool ReduceLoopStrength(Loop *L, IVUsers &IU, ScalarEvolution &SE,
     }
   }
 
-  DbgApplyEqualValues(DbgValueToEqualSet);
+  DbgApplyEqualValues(DbgValueToEqualSet, DbgValueToExpression);
 
   return Changed;
 }

diff  --git a/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp b/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
index 0cb9771b6e86..809e62b330dd 100644
--- a/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
+++ b/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
@@ -266,11 +266,13 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
   const auto AllPrecedingUsesFromBlockHoisted = [&NotHoisted](const User *U) {
     // Debug variable has special operand to check it's not hoisted.
     if (const auto *DVI = dyn_cast<DbgVariableIntrinsic>(U)) {
-      if (const auto *I =
-              dyn_cast_or_null<Instruction>(DVI->getVariableLocationOp(0)))
-        if (NotHoisted.count(I) == 0)
-          return true;
-      return false;
+      return all_of(DVI->location_ops(), [&NotHoisted](Value *V) {
+        if (const auto *I = dyn_cast_or_null<Instruction>(V)) {
+          if (NotHoisted.count(I) == 0)
+            return true;
+        }
+        return false;
+      });
     }
 
     // Usially debug label instrinsic corresponds to label in LLVM IR. In these

diff  --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index 9697e7d72c2c..d1a3ae5c0ed0 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -407,7 +407,8 @@ static bool removeRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
 /// - Keep track of non-overlapping fragments.
 static bool removeRedundantDbgInstrsUsingForwardScan(BasicBlock *BB) {
   SmallVector<DbgValueInst *, 8> ToBeRemoved;
-  DenseMap<DebugVariable, std::pair<Value *, DIExpression *> > VariableMap;
+  DenseMap<DebugVariable, std::pair<SmallVector<Value *, 4>, DIExpression *>>
+      VariableMap;
   for (auto &I : *BB) {
     if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I)) {
       DebugVariable Key(DVI->getVariable(),
@@ -416,10 +417,10 @@ static bool removeRedundantDbgInstrsUsingForwardScan(BasicBlock *BB) {
       auto VMI = VariableMap.find(Key);
       // Update the map if we found a new value/expression describing the
       // variable, or if the variable wasn't mapped already.
-      if (VMI == VariableMap.end() ||
-          VMI->second.first != DVI->getValue() ||
+      SmallVector<Value *, 4> Values(DVI->getValues());
+      if (VMI == VariableMap.end() || VMI->second.first != Values ||
           VMI->second.second != DVI->getExpression()) {
-        VariableMap[Key] = { DVI->getValue(), DVI->getExpression() };
+        VariableMap[Key] = {Values, DVI->getExpression()};
         continue;
       }
       // Found an identical mapping. Remember the instruction for later removal.

diff  --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index 2b0ee77f4c9b..4ad33c14585d 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -1511,20 +1511,19 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
       continue;
     }
 
-    // If the location isn't a constant or an instruction, delete the
-    // intrinsic.
-    auto *DVI = cast<DbgVariableIntrinsic>(DII);
-    Value *Location = DVI->getVariableLocationOp(0);
-    if (!Location ||
-        (!isa<Constant>(Location) && !isa<Instruction>(Location))) {
-      DebugIntrinsicsToDelete.push_back(DVI);
-      continue;
-    }
+    auto IsInvalidLocation = [&NewFunc](Value *Location) {
+      // Location is invalid if it isn't a constant or an instruction, or is an
+      // instruction but isn't in the new function.
+      if (!Location ||
+          (!isa<Constant>(Location) && !isa<Instruction>(Location)))
+        return true;
+      Instruction *LocationInst = dyn_cast<Instruction>(Location);
+      return LocationInst && LocationInst->getFunction() != &NewFunc;
+    };
 
-    // If the variable location is an instruction but isn't in the new
-    // function, delete the intrinsic.
-    Instruction *LocationInst = dyn_cast<Instruction>(Location);
-    if (LocationInst && LocationInst->getFunction() != &NewFunc) {
+    auto *DVI = cast<DbgVariableIntrinsic>(DII);
+    // If any of the used locations are invalid, delete the intrinsic.
+    if (any_of(DVI->location_ops(), IsInvalidLocation)) {
       DebugIntrinsicsToDelete.push_back(DVI);
       continue;
     }

diff  --git a/llvm/lib/Transforms/Utils/Debugify.cpp b/llvm/lib/Transforms/Utils/Debugify.cpp
index fc42f2c51648..73a3a4063f6b 100644
--- a/llvm/lib/Transforms/Utils/Debugify.cpp
+++ b/llvm/lib/Transforms/Utils/Debugify.cpp
@@ -554,15 +554,16 @@ bool diagnoseMisSizedDbgValue(Module &M, DbgValueInst *DVI) {
   //
   // TODO: This, along with a check for non-null value operands, should be
   // promoted to verifier failures.
-  Value *V = DVI->getValue();
-  if (!V)
-    return false;
 
   // For now, don't try to interpret anything more complicated than an empty
   // DIExpression. Eventually we should try to handle OP_deref and fragments.
   if (DVI->getExpression()->getNumElements())
     return false;
 
+  Value *V = DVI->getVariableLocationOp(0);
+  if (!V)
+    return false;
+
   Type *Ty = V->getType();
   uint64_t ValueOperandSize = getAllocSizeInBits(M, Ty);
   Optional<uint64_t> DbgVarSize = DVI->getFragmentSizeInBits();

diff  --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index e899bb13a41a..e1ea7c8e27a9 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -411,7 +411,7 @@ bool llvm::wouldInstructionBeTriviallyDead(Instruction *I,
     return true;
   }
   if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(I)) {
-    if (DVI->getValue())
+    if (DVI->hasArgList() || DVI->getValue(0))
       return false;
     return true;
   }
@@ -1360,7 +1360,7 @@ static bool PhiHasDebugValue(DILocalVariable *DIVar,
   SmallVector<DbgValueInst *, 1> DbgValues;
   findDbgValues(DbgValues, APN);
   for (auto *DVI : DbgValues) {
-    assert(DVI->getValue() == APN);
+    assert(is_contained(DVI->getValues(), APN));
     if ((DVI->getVariable() == DIVar) && (DVI->getExpression() == DIExpr))
       return true;
   }
@@ -1387,13 +1387,19 @@ static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
-  if (DII->isAddressOfVariable())
-    if (auto *AI = dyn_cast_or_null<AllocaInst>(DII->getVariableLocationOp(0)))
+  if (DII->isAddressOfVariable()) {
+    // DII should have exactly 1 location when it is an address.
+    assert(DII->getNumVariableLocationOps() == 1 &&
+           "address of variable must have exactly 1 location operand.");
+    if (auto *AI =
+            dyn_cast_or_null<AllocaInst>(DII->getVariableLocationOp(0))) {
       if (Optional<TypeSize> FragmentSize = AI->getAllocationSizeInBits(DL)) {
         assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
                "Both sizes should agree on the scalable flag.");
         return TypeSize::isKnownGE(ValueSize, *FragmentSize);
       }
+    }
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }
@@ -1596,17 +1602,26 @@ void llvm::insertDebugValuesForPHIs(BasicBlock *BB,
   ValueToValueMapTy DbgValueMap;
   for (auto &I : *BB) {
     if (auto DbgII = dyn_cast<DbgVariableIntrinsic>(&I)) {
-      if (auto *Loc =
-              dyn_cast_or_null<PHINode>(DbgII->getVariableLocationOp(0)))
-        DbgValueMap.insert({Loc, DbgII});
+      for (Value *V : DbgII->location_ops())
+        if (auto *Loc = dyn_cast_or_null<PHINode>(V))
+          DbgValueMap.insert({Loc, DbgII});
     }
   }
   if (DbgValueMap.size() == 0)
     return;
 
+  // Map a pair of the destination BB and old dbg.value to the new dbg.value,
+  // so that if a dbg.value is being rewritten to use more than one of the
+  // inserted PHIs in the same destination BB, we can update the same dbg.value
+  // with all the new PHIs instead of creating one copy for each.
+  MapVector<std::pair<BasicBlock *, DbgVariableIntrinsic *>,
+            DbgVariableIntrinsic *>
+      NewDbgValueMap;
   // Then iterate through the new PHIs and look to see if they use one of the
-  // previously mapped PHIs. If so, insert a new dbg.value intrinsic that will
-  // propagate the info through the new PHI.
+  // previously mapped PHIs. If so, create a new dbg.value intrinsic that will
+  // propagate the info through the new PHI. If we use more than one new PHI in
+  // a single destination BB with the same old dbg.value, merge the updates so
+  // that we get a single new dbg.value with all the new PHIs.
   for (auto PHI : InsertedPHIs) {
     BasicBlock *Parent = PHI->getParent();
     // Avoid inserting an intrinsic into an EH block.
@@ -1616,15 +1631,27 @@ void llvm::insertDebugValuesForPHIs(BasicBlock *BB,
       auto V = DbgValueMap.find(VI);
       if (V != DbgValueMap.end()) {
         auto *DbgII = cast<DbgVariableIntrinsic>(V->second);
-        DbgVariableIntrinsic *NewDbgII =
-            cast<DbgVariableIntrinsic>(DbgII->clone());
-        NewDbgII->replaceVariableLocationOp(VI, PHI);
-        auto InsertionPt = Parent->getFirstInsertionPt();
-        assert(InsertionPt != Parent->end() && "Ill-formed basic block");
-        NewDbgII->insertBefore(&*InsertionPt);
+        auto NewDI = NewDbgValueMap.find({Parent, DbgII});
+        if (NewDI == NewDbgValueMap.end()) {
+          auto *NewDbgII = cast<DbgVariableIntrinsic>(DbgII->clone());
+          NewDI = NewDbgValueMap.insert({{Parent, DbgII}, NewDbgII}).first;
+        }
+        DbgVariableIntrinsic *NewDbgII = NewDI->second;
+        // If PHI contains VI as an operand more than once, we may
+        // replaced it in NewDbgII; confirm that it is present.
+        if (is_contained(NewDbgII->location_ops(), VI))
+          NewDbgII->replaceVariableLocationOp(VI, PHI);
       }
     }
   }
+  // Insert thew new dbg.values into their destination blocks.
+  for (auto DI : NewDbgValueMap) {
+    BasicBlock *Parent = DI.first.first;
+    auto *NewDbgII = DI.second;
+    auto InsertionPt = Parent->getFirstInsertionPt();
+    assert(InsertionPt != Parent->end() && "Ill-formed basic block");
+    NewDbgII->insertBefore(&*InsertionPt);
+  }
 }
 
 /// Finds all intrinsics declaring local variables as living in the memory that
@@ -1665,11 +1692,25 @@ void llvm::findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues, Value *V) {
   // DenseMap lookup.
   if (!V->isUsedByMetadata())
     return;
-  if (auto *L = LocalAsMetadata::getIfExists(V))
-    if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L))
+  // TODO: If this value appears multiple times in a DIArgList, we should still
+  // only add the owning DbgValueInst once; use this set to track ArgListUsers.
+  // This behaviour can be removed when we can automatically remove duplicates.
+  SmallPtrSet<DbgValueInst *, 4> EncounteredDbgValues;
+  if (auto *L = LocalAsMetadata::getIfExists(V)) {
+    if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L)) {
       for (User *U : MDV->users())
         if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(U))
           DbgValues.push_back(DVI);
+    }
+    for (Metadata *AL : L->getAllArgListUsers()) {
+      if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), AL)) {
+        for (User *U : MDV->users())
+          if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(U))
+            if (EncounteredDbgValues.insert(DVI).second)
+              DbgValues.push_back(DVI);
+      }
+    }
+  }
 }
 
 void llvm::findDbgUsers(SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers,
@@ -1678,11 +1719,25 @@ void llvm::findDbgUsers(SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers,
   // DenseMap lookup.
   if (!V->isUsedByMetadata())
     return;
-  if (auto *L = LocalAsMetadata::getIfExists(V))
-    if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L))
+  // TODO: If this value appears multiple times in a DIArgList, we should still
+  // only add the owning DbgValueInst once; use this set to track ArgListUsers.
+  // This behaviour can be removed when we can automatically remove duplicates.
+  SmallPtrSet<DbgVariableIntrinsic *, 4> EncounteredDbgValues;
+  if (auto *L = LocalAsMetadata::getIfExists(V)) {
+    if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L)) {
       for (User *U : MDV->users())
         if (DbgVariableIntrinsic *DII = dyn_cast<DbgVariableIntrinsic>(U))
           DbgUsers.push_back(DII);
+    }
+    for (Metadata *AL : L->getAllArgListUsers()) {
+      if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), AL)) {
+        for (User *U : MDV->users())
+          if (DbgVariableIntrinsic *DII = dyn_cast<DbgVariableIntrinsic>(U))
+            if (EncounteredDbgValues.insert(DII).second)
+              DbgUsers.push_back(DII);
+      }
+    }
+  }
 }
 
 bool llvm::replaceDbgDeclare(Value *Address, Value *NewAddress,
@@ -1752,9 +1807,14 @@ void llvm::salvageDebugInfoForDbgValues(
     // are implicitly pointing out the value as a DWARF memory location
     // description.
     bool StackValue = isa<DbgValueInst>(DII);
+    auto DIILocation = DII->location_ops();
+    assert(
+        is_contained(DIILocation, &I) &&
+        "DbgVariableIntrinsic must use salvaged instruction as its location");
+    unsigned LocNo = std::distance(DIILocation.begin(), find(DIILocation, &I));
 
     DIExpression *DIExpr =
-        salvageDebugInfoImpl(I, DII->getExpression(), StackValue);
+        salvageDebugInfoImpl(I, DII->getExpression(), StackValue, LocNo);
 
     // salvageDebugInfoImpl should fail on examining the first element of
     // DbgUsers, or none of them.
@@ -1847,7 +1907,7 @@ bool getSalvageOpsForBinOp(BinaryOperator *BI,
 
 DIExpression *llvm::salvageDebugInfoImpl(Instruction &I,
                                          DIExpression *SrcDIExpr,
-                                         bool WithStackValue) {
+                                         bool WithStackValue, unsigned LocNo) {
   auto &M = *I.getModule();
   auto &DL = M.getDataLayout();
 
@@ -1855,7 +1915,7 @@ DIExpression *llvm::salvageDebugInfoImpl(Instruction &I,
   auto doSalvage = [&](SmallVectorImpl<uint64_t> &Ops) -> DIExpression * {
     DIExpression *DIExpr = SrcDIExpr;
     if (!Ops.empty()) {
-      DIExpr = DIExpression::prependOpcodes(DIExpr, Ops, WithStackValue);
+      DIExpr = DIExpression::appendOpsToArg(DIExpr, Ops, LocNo, WithStackValue);
     }
     return DIExpr;
   };

diff  --git a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
index 784d0e437ba0..2c68e4b3c32e 100644
--- a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
+++ b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp
@@ -384,11 +384,14 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
     // possible or create a clone in the OldPreHeader if not.
     Instruction *LoopEntryBranch = OrigPreheader->getTerminator();
 
-    // Record all debug intrinsics preceding LoopEntryBranch to avoid duplication.
+    // Record all debug intrinsics preceding LoopEntryBranch to avoid
+    // duplication.
     using DbgIntrinsicHash =
-      std::pair<std::pair<Value *, DILocalVariable *>, DIExpression *>;
+        std::pair<std::pair<hash_code, DILocalVariable *>, DIExpression *>;
     auto makeHash = [](DbgVariableIntrinsic *D) -> DbgIntrinsicHash {
-      return {{D->getVariableLocationOp(0), D->getVariable()},
+      auto VarLocOps = D->location_ops();
+      return {{hash_combine_range(VarLocOps.begin(), VarLocOps.end()),
+               D->getVariable()},
               D->getExpression()};
     };
     SmallDenseSet<DbgIntrinsicHash, 8> DbgIntrinsics;

diff  --git a/llvm/unittests/IR/DebugInfoTest.cpp b/llvm/unittests/IR/DebugInfoTest.cpp
index e7cfce48f8cf..58936fb2b00c 100644
--- a/llvm/unittests/IR/DebugInfoTest.cpp
+++ b/llvm/unittests/IR/DebugInfoTest.cpp
@@ -183,7 +183,8 @@ TEST(MetadataTest, DeleteInstUsedByDbgValue) {
 
   // Delete %b. The dbg.value should now point to undef.
   I.eraseFromParent();
-  EXPECT_TRUE(isa<UndefValue>(DVIs[0]->getValue()));
+  EXPECT_EQ(DVIs[0]->getNumVariableLocationOps(), 1u);
+  EXPECT_TRUE(isa<UndefValue>(DVIs[0]->getValue(0)));
 }
 
 TEST(DIBuilder, CreateFortranArrayTypeWithAttributes) {


        


More information about the llvm-commits mailing list