Recover debug intrinsics when killing duplicate or empty basic blocks

[StephenTozer]

Summary

Fixes the bug: https://bugs.llvm.org/show_bug.cgi?id=39339

When basic blocks are killed, either due to being empty or to being an if.then or if.else block whose complement contains identical instructions, the debug intrinsics in that block are lost. This fix attempts to recover those intrinsics by either hoisting or sinking them if possible, or replacing them in the above block with Undef values to prevent debug info from falling out-of-date.

[vsk]

llvm/lib/Transforms/Utils/Local.cpp

1046

I had to squint at this for a while to figure out why it's sufficient. Could you clear this up by writing the loop as for (auto I : *BB), then asserting isa<DbgInfoIntrinsic>(&I) || I.isTerminator()?

1052

Could you add a new helper to Local.h to share this wrap-in-undef logic with llvm::replaceDbgUsesWithUndef (above)?

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1284

Is any special/extra work needed to handle duplicate dbg.values for a DebugVariable that's already present in the UndefDVIs map? The first one that's inserted into the map is hoisted and converted to an 'undef' -- what happens to the rest?

1401

You can use the set-to-undef helper here.

[efriedma]

This seems fine, generally.

Would it be possible to salvage the debug info using the branch condition? Maybe complicated, but it seems like it should be possible, at least in theory. (Probably better to leave that for a followup, though.)

[rnk]

llvm/include/llvm/IR/DebugInfoMetadata.h

3253

This is a good idea whose time has come. :) I think in the long run, we should make this concept more explicit and first-class in our IR representation, rather than something we work out by looking at fragments.

[jmorse]

llvm/lib/CodeGen/LiveDebugValues.cpp

169

NB, the current patch doesn't apply to trunk/master. This line (169) and the constructor on 195 were removed in r373727

[StephenTozer]

Updating according to review comments; the code in Local.cpp needs further work, but is close to its likely final state. This version also applies to master, so diffing the latest revision with the previous will give a lot of noise.

[aprantl]

llvm/include/llvm/IR/DebugInfoMetadata.h

3252

That comment is a bit vague for DebugInfoMetadata.

Does this represent exactly one non-overlapping fragment of an inlined instance of a variable? We should say so. Since this isn't used anywhere in this file, there should be an example for when to use this.

3253

Can you break this out into a separate NFC commit with a unit test?

[aprantl]

llvm/include/llvm/IR/DebugInfoMetadata.h

3253

@Orlando cf. the discussion in https://reviews.llvm.org/D70121

[StephenTozer]

llvm/include/llvm/IR/DebugInfoMetadata.h

3253

What sort of test coverage would be appropriate for a class like this?

[jmorse]

NB, you'll also want to remove the DebugVariable portions of this patch and assign it as a child revision of D70486, and the tests went missing in the latest revision :o)

Adrian wrote:

aprantl added a reviewer: jmorse.

Full disclosure, Stephen is also a Sony-ite, and I believe organisation-internal approves are generally frowned on. I'm happy with the general direction and implementation of this patch though.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

805–807 ↗

(On Diff #230090)

Something that accidentally snuck in?

llvm/lib/Transforms/Utils/Local.cpp

1047

While building with this patch elsewhere I ran into some assertions, from isel observing dbg.declare's with Undef operands. This is actually great: it means that beforehand we weren't just losing dbg.values, but dbg.declares describing un-SROA'd/un-mem2reg'd locations too.

Those dbg.declares need to have their operands preserved rather than set to undef -- they're not control flow dependent, so it doesn't matters where they get placed (I think, 90%).

[StephenTozer]

Show correct changeset, preserve operands for dbg.declares

[djtodoro]

Please make the stack of patches (the D70486 should be parent for this one).

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1398

clang-format please

[djtodoro]

I think the patches in the stack should be in the opposite direction (firstly D70486 and then D70486).

[StephenTozer]

Fix formatting, include whole files in patch (mistakenly removed in prior revision).

[probinson]

In D70318#1753483, @jmorse wrote:

Adrian wrote:

aprantl added a reviewer: jmorse.

Full disclosure, Stephen is also a Sony-ite, and I believe organisation-internal approves are generally frowned on. I'm happy with the general direction and implementation of this patch though.

@jmorse you demonstrably have experience in this area, so it's entirely appropriate for you to participate in the upstream reviews. Just don't let it go to your head.

[aprantl]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

44

Does this get easier to read if you factor out the two while loops into a sub-lambda and give that a descriptive name?

[StephenTozer]

Use another lambda to simplify skipping of dbg info, fix incorrect type usage in setDbgVariableUndef

[StephenTozer]

Maintain order of debug intrinsics in empty block elimination, fix test

[StephenTozer]

Use Undef int1 when undef-ing a DbgVariable without a type.

llvm/include/llvm/IR/DebugInfoMetadata.h

#endif
   Metadata *getRawFile() const { return getOperand(0); }
   Metadata *getRawElements() const { return getOperand(1); }
 
   static bool classof(const Metadata *MD) {
     return MD->getMetadataID() == DIMacroFileKind;
   }
 };
 
+/// Storage for identifying a potentially inlined instance of a variable,
+/// or a fragment thereof.
+class DebugVariable {
+  using FragmentInfo = DIExpression::FragmentInfo;
+  using OptFragmentInfo = Optional<FragmentInfo>;
+
+  const DILocalVariable *Variable;
+  OptFragmentInfo Fragment;
+  const DILocation *InlinedAt;
+
+  /// Fragment that will overlap all other fragments. Used as default when
+  /// caller demands a fragment.
+  static const FragmentInfo DefaultFragment;
+
+public:
+  DebugVariable(const DILocalVariable *Var, OptFragmentInfo &&FragmentInfo,
+                const DILocation *InlinedAt)
+      : Variable(Var), Fragment(FragmentInfo), InlinedAt(InlinedAt) {}
+
+  DebugVariable(const DILocalVariable *Var, OptFragmentInfo &FragmentInfo,
+                const DILocation *InlinedAt)
+      : Variable(Var), Fragment(FragmentInfo), InlinedAt(InlinedAt) {}
+
+  DebugVariable(const DILocalVariable *Var, const DIExpression *DIExpr,
+                const DILocation *InlinedAt)
+      : DebugVariable(Var, DIExpr->getFragmentInfo(), InlinedAt) {}
+
+  const DILocalVariable *getVar() const { return Variable; }
+  const OptFragmentInfo &getFragment() const { return Fragment; }
+  const DILocation *getInlinedAt() const { return InlinedAt; }
+
+  const FragmentInfo getFragmentDefault() const {
+    return Fragment.getValueOr(DefaultFragment);
+  }
+
+  static bool isFragmentDefault(const FragmentInfo &F) {
+    return F == DefaultFragment;
+  }
+
+  bool operator==(const DebugVariable &Other) const {
+    return std::tie(Variable, Fragment, InlinedAt) ==
+           std::tie(Other.Variable, Other.Fragment, Other.InlinedAt);
+  }
+
+  bool operator<(const DebugVariable &Other) const {
+    return std::tie(Variable, Fragment, InlinedAt) <
+           std::tie(Other.Variable, Other.Fragment, Other.InlinedAt);
+  }
+};
+
+template <> struct DenseMapInfo<DebugVariable> {
+  using DV = DebugVariable;
+  using OptFragmentInfo = Optional<DIExpression::FragmentInfo>;
+  using FragmentInfo = DIExpression::FragmentInfo;
+
+  // Empty key: no key should be generated that has no DILocalVariable.
+  static inline DV getEmptyKey() {
+    return DV(nullptr, OptFragmentInfo(), nullptr);
+  }
+
+  // Difference in tombstone is that the Optional is meaningful
+  static inline DV getTombstoneKey() {
+    return DV(nullptr, OptFragmentInfo({0, 0}), nullptr);
+  }
+
+  static unsigned getHashValue(const DV &D) {
+    unsigned HV = 0;
+    const OptFragmentInfo &Fragment = D.getFragment();
+    if (Fragment)
+      HV = DenseMapInfo<FragmentInfo>::getHashValue(*Fragment);
+
+    return hash_combine(D.getVar(), HV, D.getInlinedAt());
+  }
+
+  static bool isEqual(const DV &A, const DV &B) { return A == B; }
+};
+
 } // end namespace llvm
 
 #undef DEFINE_MDNODE_GET_UNPACK_IMPL
 #undef DEFINE_MDNODE_GET_UNPACK
 #undef DEFINE_MDNODE_GET
 
 #endif // LLVM_IR_DEBUGINFOMETADATA_H

llvm/lib/CodeGen/LiveDebugValues.cpp

bool dominates(MachineBasicBlock *MBB) {
         LS.getMachineBasicBlocks(DL, LBlocks);
       return LBlocks.count(MBB) != 0 || LS.dominates(DL, MBB);
     }
   };
 
   using FragmentInfo = DIExpression::FragmentInfo;
   using OptFragmentInfo = Optional<DIExpression::FragmentInfo>;
 
-  /// Storage for identifying a potentially inlined instance of a variable,
-  /// or a fragment thereof.
-  class DebugVariable {
-    const DILocalVariable *Variable;
-    OptFragmentInfo Fragment;
-    const DILocation *InlinedAt;
-
-    /// Fragment that will overlap all other fragments. Used as default when
-    /// caller demands a fragment.
-    static const FragmentInfo DefaultFragment;
-
-  public:
-    DebugVariable(const DILocalVariable *Var, OptFragmentInfo &&FragmentInfo,
-                  const DILocation *InlinedAt)
-        : Variable(Var), Fragment(FragmentInfo), InlinedAt(InlinedAt) {}
-
-    DebugVariable(const DILocalVariable *Var, OptFragmentInfo &FragmentInfo,
-                  const DILocation *InlinedAt)
-        : Variable(Var), Fragment(FragmentInfo), InlinedAt(InlinedAt) {}
-
-    DebugVariable(const DILocalVariable *Var, const DIExpression *DIExpr,
-                  const DILocation *InlinedAt)
-        : DebugVariable(Var, DIExpr->getFragmentInfo(), InlinedAt) {}
-
-    DebugVariable(const MachineInstr &MI)
-        : DebugVariable(MI.getDebugVariable(),
-                        MI.getDebugExpression()->getFragmentInfo(),
-                        MI.getDebugLoc()->getInlinedAt()) {}
-
-    const DILocalVariable *getVar() const { return Variable; }
-    const OptFragmentInfo &getFragment() const { return Fragment; }
-    const DILocation *getInlinedAt() const { return InlinedAt; }
-
-    const FragmentInfo getFragmentDefault() const {
-      return Fragment.getValueOr(DefaultFragment);
-    }
-
-    static bool isFragmentDefault(FragmentInfo &F) {
-      return F == DefaultFragment;
-    }
-
-    bool operator==(const DebugVariable &Other) const {
-      return std::tie(Variable, Fragment, InlinedAt) ==
-             std::tie(Other.Variable, Other.Fragment, Other.InlinedAt);
-    }
-
-    bool operator<(const DebugVariable &Other) const {
-      return std::tie(Variable, Fragment, InlinedAt) <
-             std::tie(Other.Variable, Other.Fragment, Other.InlinedAt);
-    }
-  };
-
-  friend struct llvm::DenseMapInfo<DebugVariable>;
-
   /// A pair of debug variable and value location.
   struct VarLoc {
     // The location at which a spilled variable resides. It consists of a
     // register and an offset.
     struct SpillLoc {
       unsigned SpillBase;
       int SpillOffset;
       bool operator==(const SpillLoc &Other) const {
union {
       SpillLoc SpillLocation;
       uint64_t Hash;
       int64_t Immediate;
       const ConstantFP *FPImm;
       const ConstantInt *CImm;
     } Loc;
 
     VarLoc(const MachineInstr &MI, LexicalScopes &LS,
            VarLocKind K = InvalidKind)
-        : Var(MI), Expr(MI.getDebugExpression()), MI(MI),
-          UVS(MI.getDebugLoc(), LS) {
+        : Var(MI.getDebugVariable(), MI.getDebugExpression(),
+              MI.getDebugLoc()->getInlinedAt()),
+          Expr(MI.getDebugExpression()), MI(MI), UVS(MI.getDebugLoc(), LS) {
       static_assert((sizeof(Loc) == sizeof(uint64_t)),
                     "hash does not cover all members of Loc");
       assert(MI.isDebugValue() && "not a DBG_VALUE");
       assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
       if (int RegNo = isDbgValueDescribedByReg(MI)) {
         Kind = MI.isDebugEntryValue() ? EntryValueKind : RegisterKind;
         Loc.RegNo = RegNo;
       } else if (MI.getOperand(0).isImm()) {
         Kind = ImmediateKind;
         Loc.Immediate = MI.getOperand(0).getImm();
       } else if (MI.getOperand(0).isFPImm()) {
         Kind = ImmediateKind;
         Loc.FPImm = MI.getOperand(0).getFPImm();
       } else if (MI.getOperand(0).isCImm()) {
         Kind = ImmediateKind;
         Loc.CImm = MI.getOperand(0).getCImm();
       }
       assert((Kind != ImmediateKind || !MI.isDebugEntryValue()) &&
              "entry values must be register locations");
     }
 
     /// The constructor for spill locations.
     VarLoc(const MachineInstr &MI, unsigned SpillBase, int SpillOffset,
            LexicalScopes &LS, const MachineInstr &OrigMI)
-        : Var(MI), Expr(MI.getDebugExpression()), MI(OrigMI),
-          UVS(MI.getDebugLoc(), LS) {
+        : Var(MI.getDebugVariable(), MI.getDebugExpression(),
+              MI.getDebugLoc()->getInlinedAt()),
+          Expr(MI.getDebugExpression()), MI(OrigMI), UVS(MI.getDebugLoc(), LS) {
       assert(MI.isDebugValue() && "not a DBG_VALUE");
       assert(MI.getNumOperands() == 4 && "malformed DBG_VALUE");
       Kind = SpillLocKind;
       Loc.SpillLocation = {SpillBase, SpillOffset};
     }
 
     // Is the Loc field a constant or constant object?
     bool isConstant() const { return Kind == ImmediateKind; }
void printVarLocInMBB(const MachineFunction &MF, const VarLocInMBB &V,
                         raw_ostream &Out) const;
 
   /// Calculate the liveness information for the given machine function.
   bool runOnMachineFunction(MachineFunction &MF) override;
 };
 
 } // end anonymous namespace
 
-namespace llvm {
-
-template <> struct DenseMapInfo<LiveDebugValues::DebugVariable> {
-  using DV = LiveDebugValues::DebugVariable;
-  using OptFragmentInfo = LiveDebugValues::OptFragmentInfo;
-  using FragmentInfo = LiveDebugValues::FragmentInfo;
-
-  // Empty key: no key should be generated that has no DILocalVariable.
-  static inline DV getEmptyKey() {
-    return DV(nullptr, OptFragmentInfo(), nullptr);
-  }
-
-  // Difference in tombstone is that the Optional is meaningful
-  static inline DV getTombstoneKey() {
-    return DV(nullptr, OptFragmentInfo({0, 0}), nullptr);
-  }
-
-  static unsigned getHashValue(const DV &D) {
-    unsigned HV = 0;
-    const OptFragmentInfo &Fragment = D.getFragment();
-    if (Fragment)
-      HV = DenseMapInfo<FragmentInfo>::getHashValue(*Fragment);
-
-    return hash_combine(D.getVar(), HV, D.getInlinedAt());
-  }
-
-  static bool isEqual(const DV &A, const DV &B) { return A == B; }
-};
-
-} // namespace llvm
-
 //===----------------------------------------------------------------------===//
 //            Implementation
 //===----------------------------------------------------------------------===//
 
-const DIExpression::FragmentInfo
-    LiveDebugValues::DebugVariable::DefaultFragment = {
-        std::numeric_limits<uint64_t>::max(),
-        std::numeric_limits<uint64_t>::min()};
-
 char LiveDebugValues::ID = 0;
 
 char &llvm::LiveDebugValuesID = LiveDebugValues::ID;
 
 INITIALIZE_PASS(LiveDebugValues, DEBUG_TYPE, "Live DEBUG_VALUE analysis",
                 false, false)
 
 /// Default construct and initialize the pass.
void LiveDebugValues::insertTransferDebugPair(
   MachineFunction *MF = MI.getParent()->getParent();
   MachineInstr *NewDebugInstr;
 
   auto ProcessVarLoc = [&MI, &OpenRanges, &Transfers, &DebugInstr,
                         &VarLocIDs](VarLoc &VL, MachineInstr *NewDebugInstr) {
     unsigned LocId = VarLocIDs.insert(VL);
 
     // Close this variable's previous location range.
-    DebugVariable V(*DebugInstr);
+    DebugVariable V(DebugInstr->getDebugVariable(),
+                    DebugInstr->getDebugExpression(),
+                    DebugInstr->getDebugLoc()->getInlinedAt());
     OpenRanges.erase(V);
 
     OpenRanges.insert(LocId, VL.Var);
     // The newly created DBG_VALUE instruction NewDebugInstr must be inserted
     // after MI. Keep track of the pairing.
     TransferDebugPair MIP = {&MI, NewDebugInstr};
     Transfers.push_back(MIP);
   };

 ///           fragment usage.
 /// \param SeenFragments Map from DILocalVariable to all fragments of that
 ///           Variable which are known to exist.
 /// \param OverlappingFragments The overlap map being constructed, from one
 ///           Var/Fragment pair to a vector of fragments known to overlap.
 void LiveDebugValues::accumulateFragmentMap(MachineInstr &MI,
                                             VarToFragments &SeenFragments,
                                             OverlapMap &OverlappingFragments) {
-  DebugVariable MIVar(MI);
+  DebugVariable MIVar(MI.getDebugVariable(), MI.getDebugExpression(),
+                      MI.getDebugLoc()->getInlinedAt());
   FragmentInfo ThisFragment = MIVar.getFragmentDefault();
 
   // If this is the first sighting of this variable, then we are guaranteed
   // there are currently no overlapping fragments either. Initialize the set
   // of seen fragments, record no overlaps for the current one, and return.
   auto SeenIt = SeenFragments.find(MIVar.getVar());
   if (SeenIt == SeenFragments.end()) {
     SmallSet<FragmentInfo, 4> OneFragment;

llvm/lib/IR/DebugInfoMetadata.cpp

 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instructions.h"
 
 #include <numeric>
 
 using namespace llvm;
 
+const DIExpression::FragmentInfo DebugVariable::DefaultFragment = {
+    std::numeric_limits<uint64_t>::max(), std::numeric_limits<uint64_t>::min()};
+
 DILocation::DILocation(LLVMContext &C, StorageType Storage, unsigned Line,
                        unsigned Column, ArrayRef<Metadata *> MDs,
                        bool ImplicitCode)
     : MDNode(C, DILocationKind, Storage, MDs) {
   assert((MDs.size() == 1 || MDs.size() == 2) &&
          "Expected a scope and optional inlined-at");
 
   // Set line and column.

llvm/lib/Transforms/Utils/Local.cpp

if (Succ->getSinglePredecessor()) {
     Succ->getInstList().splice(Succ->getFirstNonPHI()->getIterator(),
                                BB->getInstList());
   } else {
     while (PHINode *PN = dyn_cast<PHINode>(&BB->front())) {
       // We explicitly check for such uses in CanPropagatePredecessorsForPHIs.
       assert(PN->use_empty() && "There shouldn't be any uses here!");
       PN->eraseFromParent();
     }
+    // If Succ has multiple predecessors, each debug intrinsic in BB may or may
+    // not be valid when we reach Succ, so the debug variable should be set
+    // undef since its value is unknown.
+    for (auto DI = BB->begin(); isa<DbgInfoIntrinsic>(DI); ++DI)
+      if (auto DVI = cast<DbgVariableIntrinsic>(DI)) {
+        Value *V = DVI->getVariableLocation();
+        DVI->setArgOperand(
+            0, MetadataAsValue::get(
+                   DVI->getContext(),
+                   ValueAsMetadata::get(UndefValue::get(V->getType()))));
+        DVI->moveBefore(Succ->getFirstNonPHI());
+      }
   }
 
   // If the unconditional branch we replaced contains llvm.loop metadata, we
   // add the metadata to the branch instructions in the predecessors.
   unsigned LoopMDKind = BB->getContext().getMDKindID("llvm.loop");
   Instruction *TI = BB->getTerminator();
   if (TI)
     if (MDNode *LoopMD = TI->getMetadata(LoopMDKind))

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

 //===- SimplifyCFG.cpp - Code to perform CFG simplification ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Peephole optimize the CFG.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/MemorySSA.h"
 #include "llvm/Analysis/MemorySSAUpdater.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/ConstantRange.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/LLVMContext.h"
static bool HoistThenElseCodeToIf(BranchInst *BI,
   BasicBlock *BB1 = BI->getSuccessor(0); // The true destination.
   BasicBlock *BB2 = BI->getSuccessor(1); // The false destination
 
   BasicBlock::iterator BB1_Itr = BB1->begin();
   BasicBlock::iterator BB2_Itr = BB2->begin();
 
   Instruction *I1 = &*BB1_Itr++, *I2 = &*BB2_Itr++;
   // Skip debug info if it is not identical.
+
+  // If the terminator instruction is hoisted, and any variable locations have
+  // non-identical debug intrinsics, then those variable locations must be set
+  // as undef.
+  // FIXME: If each block contains identical debug variable intrinsics in a
+  // different order, they will be considered non-identical and be dropped.
+  MapVector<DebugVariable, DbgVariableIntrinsic *> UndefDVIs;
+
+  auto SkipNonIdenticalDbgInfo =
+      [&BB1_Itr, &BB2_Itr, &UndefDVIs](Instruction *&I1, Instruction *&I2) {
         DbgInfoIntrinsic *DBI1 = dyn_cast<DbgInfoIntrinsic>(I1);
         DbgInfoIntrinsic *DBI2 = dyn_cast<DbgInfoIntrinsic>(I2);
         if (!DBI1 || !DBI2 || !DBI1->isIdenticalToWhenDefined(DBI2)) {
-    while (isa<DbgInfoIntrinsic>(I1))
+          while (isa<DbgInfoIntrinsic>(I1)) {
+            if (DbgVariableIntrinsic *DVI = dyn_cast<DbgVariableIntrinsic>(I1))
+              UndefDVIs.insert(
+                  {DebugVariable(DVI->getVariable(), DVI->getExpression(),
+                                 DVI->getDebugLoc()->getInlinedAt()),
+                   DVI});
             I1 = &*BB1_Itr++;
-    while (isa<DbgInfoIntrinsic>(I2))
+          }
+          while (isa<DbgInfoIntrinsic>(I2)) {
+            if (DbgVariableIntrinsic *DVI = dyn_cast<DbgVariableIntrinsic>(I2))
+              UndefDVIs.insert(
+                  {DebugVariable(DVI->getVariable(), DVI->getExpression(),
+                                 DVI->getDebugLoc()->getInlinedAt()),
+                   DVI});
             I2 = &*BB2_Itr++;
           }
+        }
+      };
+
+  SkipNonIdenticalDbgInfo(I1, I2);
+
   // FIXME: Can we define a safety predicate for CallBr?
   if (isa<PHINode>(I1) || !I1->isIdenticalToWhenDefined(I2) ||
       (isa<InvokeInst>(I1) && !isSafeToHoistInvoke(BB1, BB2, I1, I2)) ||
       isa<CallBrInst>(I1))
     return false;
 
   BasicBlock *BIParent = BI->getParent();
 
if (isa<DbgInfoIntrinsic>(I1) || isa<DbgInfoIntrinsic>(I2)) {
       I1->applyMergedLocation(I1->getDebugLoc(), I2->getDebugLoc());
 
       I2->eraseFromParent();
       Changed = true;
     }
 
     I1 = &*BB1_Itr++;
     I2 = &*BB2_Itr++;
-    // Skip debug info if it is not identical.
-    DbgInfoIntrinsic *DBI1 = dyn_cast<DbgInfoIntrinsic>(I1);
-    DbgInfoIntrinsic *DBI2 = dyn_cast<DbgInfoIntrinsic>(I2);
-    if (!DBI1 || !DBI2 || !DBI1->isIdenticalToWhenDefined(DBI2)) {
-      while (isa<DbgInfoIntrinsic>(I1))
-        I1 = &*BB1_Itr++;
-      while (isa<DbgInfoIntrinsic>(I2))
-        I2 = &*BB2_Itr++;
-    }
+    SkipNonIdenticalDbgInfo(I1, I2);
   } while (I1->isIdenticalToWhenDefined(I2));
 
   return true;
 
 HoistTerminator:
   // It may not be possible to hoist an invoke.
   // FIXME: Can we define a safety predicate for CallBr?
   if (isa<InvokeInst>(I1) && !isSafeToHoistInvoke(BB1, BB2, I1, I2))
for (PHINode &PN : Succ->phis()) {
       if (isa<ConstantExpr>(BB1V) && !isSafeToSpeculativelyExecute(BB1V))
         return Changed;
       if (isa<ConstantExpr>(BB2V) && !isSafeToSpeculativelyExecute(BB2V))
         return Changed;
     }
   }
 
   // Okay, it is safe to hoist the terminator.
+  for (auto DIVariableInst : UndefDVIs) {
+    DbgVariableIntrinsic *DVI = DIVariableInst.second;
+    DVI->moveBefore(BI);
+    if (Value *V = DVI->getVariableLocation()) {
+      DVI->setArgOperand(
+          0, MetadataAsValue::get(
+                 DVI->getContext(),
+                 ValueAsMetadata::get(UndefValue::get(V->getType()))));
+    }
+  }
+
   Instruction *NT = I1->clone();
   BIParent->getInstList().insert(BI->getIterator(), NT);
   if (!NT->getType()->isVoidTy()) {
     I1->replaceAllUsesWith(NT);
     I2->replaceAllUsesWith(NT);
     NT->takeName(I1);
   }
 

llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-else.ll

+; RUN: opt -simplifycfg -S < %s | FileCheck %s
+; Checks that when the if.then and if.else blocks are both eliminated by
+; SimplifyCFG, as the common code is hoisted out, the variables with a
+; dbg.value in either of those blocks are set undef just before the
+; conditional branch instruction.
+
+define i32 @"?fn@@YAHH@Z"(i32 %foo) !dbg !8 {
+; CHECK-LABEL: entry:
+entry:
+  call void @llvm.dbg.value(metadata i32 %foo, metadata !13, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata i32 0, metadata !14, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata i32 0, metadata !15, metadata !DIExpression()), !dbg !16
+  %cmp = icmp eq i32 %foo, 4, !dbg !17
+; CHECK: call void @llvm.dbg.value(metadata i32 undef, metadata [[BEARDS:![0-9]+]]
+; CHECK: call void @llvm.dbg.value(metadata i32 undef, metadata [[BIRDS:![0-9]+]]
+  br i1 %cmp, label %if.then, label %if.else, !dbg !17
+
+if.then:                                          ; preds = %entry
+  call void @llvm.dbg.value(metadata i32 8, metadata !14, metadata !DIExpression()), !dbg !16
+  br label %if.end, !dbg !18
+
+if.else:                                          ; preds = %entry
+  call void @llvm.dbg.value(metadata i32 4, metadata !14, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata i32 8, metadata !15, metadata !DIExpression()), !dbg !16
+  br label %if.end, !dbg !21
+
+if.end:                                           ; preds = %if.else, %if.then
+  %beards.0 = phi i32 [ 8, %if.then ], [ 4, %if.else ], !dbg !23
+  call void @llvm.dbg.value(metadata i32 %beards.0, metadata !14, metadata !DIExpression()), !dbg !16
+  ret i32 %beards.0, !dbg !24
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+; CHECK-LABEL: }
+; CHECK: [[BEARDS]] = !DILocalVariable(name: "beards"
+; CHECK: [[BIRDS]] = !DILocalVariable(name: "birds"
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 10.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "C:\5Cdev\5Cllvm-project", checksumkind: CSK_MD5, checksum: "64604a72fdf5b6db8aa2328236bedd6b")
+!2 = !{}
+!3 = !{i32 2, !"CodeView", i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 10.0.0 "}
+!8 = distinct !DISubprogram(name: "fn", linkageName: "?fn@@YAHH@Z", scope: !1, file: !1, line: 1, type: !9, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !{!13, !14, !15}
+!13 = !DILocalVariable(name: "foo", arg: 1, scope: !8, file: !1, line: 1, type: !11)
+!14 = !DILocalVariable(name: "beards", scope: !8, file: !1, line: 2, type: !11)
+!15 = !DILocalVariable(name: "birds", scope: !8, file: !1, line: 3, type: !11)
+!16 = !DILocation(line: 0, scope: !8)
+!17 = !DILocation(line: 5, scope: !8)
+!18 = !DILocation(line: 8, scope: !19)
+!19 = distinct !DILexicalBlock(scope: !20, file: !1, line: 5)
+!20 = distinct !DILexicalBlock(scope: !8, file: !1, line: 5)
+!21 = !DILocation(line: 11, scope: !22)
+!22 = distinct !DILexicalBlock(scope: !20, file: !1, line: 8)
+!23 = !DILocation(line: 0, scope: !20)
+!24 = !DILocation(line: 13, scope: !8)

llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-empty.ll

+; RUN: opt -simplifycfg -S < %s | FileCheck %s
+; Checks that when the if.then block is eliminated due to containing no
+; instructions, the debug intrinsics are hoisted out of the block before its
+; deletion. The hoisted intrinsics should have undef values as the branch
+; behaviour is unknown to the intrinsics after hoisting.
+
+define dso_local i32 @"?fn@@YAHH@Z"(i32 %foo) local_unnamed_addr #0 !dbg !8 {
+; CHECK-LABEL: entry:
+entry:
+  call void @llvm.dbg.value(metadata i32 %foo, metadata !13, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata i32 0, metadata !14, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata i32 0, metadata !15, metadata !DIExpression()), !dbg !16
+  %cmp = icmp eq i32 %foo, 4, !dbg !17
+; CHECK: call void @llvm.dbg.value(metadata i32 undef, metadata [[BEARDS:![0-9]+]]
+; CHECK: call void @llvm.dbg.value(metadata i32 undef, metadata [[BIRDS:![0-9]+]]
+  br i1 %cmp, label %if.then, label %if.else, !dbg !17
+; CHECK-LABEL: if.else:
+
+if.then:                                          ; preds = %entry
+  call void @llvm.dbg.value(metadata i32 8, metadata !14, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata i32 3, metadata !15, metadata !DIExpression()), !dbg !16
+  br label %if.end, !dbg !18
+
+if.else:                                          ; preds = %entry
+  call void @"?side@@YAXXZ"(), !dbg !21
+  call void @llvm.dbg.value(metadata i32 4, metadata !14, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.value(metadata i32 6, metadata !15, metadata !DIExpression()), !dbg !16
+  br label %if.end, !dbg !23
+
+if.end:                                           ; preds = %if.else, %if.then
+  %beards.0 = phi i32 [ 8, %if.then ], [ 4, %if.else ], !dbg !24
+  call void @llvm.dbg.value(metadata i32 %beards.0, metadata !14, metadata !DIExpression()), !dbg !16
+  ret i32 %beards.0, !dbg !25
+}
+
+declare dso_local void @"?side@@YAXXZ"() local_unnamed_addr
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+; CHECK: [[BEARDS]] = !DILocalVariable(name: "beards"
+; CHECK: [[BIRDS]] = !DILocalVariable(name: "birds"
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 10.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test2.cpp", directory: "C:\5Cdev\5Cllvm-project", checksumkind: CSK_MD5, checksum: "8ac5d40fcc9914d6479c1a770dfdc176")
+!2 = !{}
+!3 = !{i32 2, !"CodeView", i32 1}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 2}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 10.0.0 "}
+!8 = distinct !DISubprogram(name: "fn", linkageName: "?fn@@YAHH@Z", scope: !1, file: !1, line: 3, type: !9, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !{!13, !14, !15}
+!13 = !DILocalVariable(name: "foo", arg: 1, scope: !8, file: !1, line: 3, type: !11)
+!14 = !DILocalVariable(name: "beards", scope: !8, file: !1, line: 4, type: !11)
+!15 = !DILocalVariable(name: "birds", scope: !8, file: !1, line: 5, type: !11)
+!16 = !DILocation(line: 0, scope: !8)
+!17 = !DILocation(line: 7, scope: !8)
+!18 = !DILocation(line: 10, scope: !19)
+!19 = distinct !DILexicalBlock(scope: !20, file: !1, line: 7)
+!20 = distinct !DILexicalBlock(scope: !8, file: !1, line: 7)
+!21 = !DILocation(line: 11, scope: !22)
+!22 = distinct !DILexicalBlock(scope: !20, file: !1, line: 10)
+!23 = !DILocation(line: 14, scope: !22)
+!24 = !DILocation(line: 0, scope: !20)
+!25 = !DILocation(line: 16, scope: !8)