[DebugInfo][RemoveDIs] Add conversion utilities between dbg.value form and DPValue new-debug-info form

[jmorse]

Summary

This patch contributes some more plumbing to the "removing debug intrinsics" design: specifically utility methods for converting from one format into another. As it stands, it's going to be possible for blocks to move between "dbg.value" format (intrinsics) and "DPValue" format (no intrinsics) as needs be. As a result, Modules / Functions / Blocks grow a Boolean flag to indicate which format they currently contain, and have helper methods to initiate transferring from one to the other.

There's no consideration for serialising the flag to textual IR or bitcode: this is purely an in-memory representation right now. There's a "validate" method added to blocks to check that the debug-info isn't obviously broken, this is a smoke test rather than something the Verifier needs to be rigorous about.

There are a few places where we scatter calls to set the "IsNewDbgInfoFormat" flag on block or function creation: normally the functions and blocks have the flag set when they're inserted into their parent, but in a few scenarios debug-info sensitive operations occur before that happens, therefore the flag has to be explicitly set on creation. This feels fairly dumb, but it's all in aid of migration/transition rather than a long term code decision.

The unit test added takes a function with dbg.values, converts it to the new DPValue form, and confirms that the layout of the data structures is as expected, and that we can convert back to exactly the same format.

[Orlando]

LGTM with some minor questions

llvm/include/llvm/IR/BasicBlock.h

87

I think this comment could be slightly misleading - the function appears to set the flag and change the block to the desired debug info format.

llvm/include/llvm/IR/Function.h

101–103

I think it's worth copying the comment you have in BasicBlock.h here (or redirecting to it) - that one clearly identifies what true and false indicate.

llvm/include/llvm/IR/Module.h

218

See my comment in Function.h

llvm/lib/IR/BasicBlock.cpp

33

clang-format

59

What about other debug intrinsics? What's the plan for those?

126

212

Any reason not to put in the member initialiser list?

258–260

This pattern is used in a few places. Could be worth using a filter_iterator (llvm::make_filter_range etc), similar to what instructionsWithoutDebug() does, that skips instructions without debug markers? YMMV.

e.g.

for (auto &Inst : instructionsWithDebugMarkers())

llvm/lib/IR/Verifier.cpp

2932

I think the verifier has a raw_ostream member OS; better to pass that in than always printing to stderr in validateDbgValues?

llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp

1777–1778 ↗

(On Diff #535765)

Shouldn't CloneBasicBlock be responsible for this?

llvm/unittests/IR/DebugInfoTest.cpp

820–823

As discussed in the previous patch can we please now use createMarker and friends in this function?

1156

Possibly worth confirming the other dbg.value fields are what we expect to see?

[jmorse]

Address review comments of various forms; also add the plumbing for replacing TrailingDPValues with an LLVMContext stored map of Block<=>TrailingDPValues.

[jmorse]

Looking at the history-diff, I've also added some flag-maintenance in IRMover, which I believe is involved in moving modules around. I think there were just a few code paths there where we start constructing replacement modules/functions, and have to start off with the flag being correct. (This is all cost+overhead of having the two implementations available right now).

I've also added a createMarker method for iterators, to ease the creation of TrailingDPValue records and for when we're not completely sure if we're inserting at end() or not.

llvm/lib/IR/BasicBlock.cpp

59

Later patches, basically. As this is being staged-in incrementally, we can get a totally-working implementation that only operates on dbg.values (which represent the most complicated problems to solve) and then naturally extend to dbg.declare, dbg.assign, dbg.label and pseudo probes.

258–260

I've been trying to steer clear of that for the moment, to make it harder to write things that rely on implementation details. That'll discourage us using such things like filter iterators in the optimisation passes in favour of proving APIs into/out-of the core classes that Do Things (TM). That then means we can decouple optimisations from how things are stored, making it easier to test out different storage models.

[Orlando]

LGTM

[jmorse]

Closed by rG7d77bbef4ad9230 . Note that I lost one of the hunks in IRMover for assigning IsNewDbgInfoFormat as that was leading to some tests assertions where slightly-incompatible objects are linked between IR files (link-global-to-func.ll, which should fail, not assert). This hunk will instead appear in the next patch series once I'm confident enough there aren't any unintended side-effects.

[jmorse]

I'm slightly surprised to see that this is regressing -g performance a bit [0], I'd thought I'd kept all these patches from actually being _used_ by LLVM until later ones land and switches are enabled. Two possibilities to my mind:

• It's the linear walk over the blocks instructions to clear DPMarkers at the end. That's something we can polish later, and #ifdef guard for now.
• Something in the metadata maps added in D153990 is getting slower? If so that must be down to branching?

All of this is #ifdef guardable while we're getting this prototype in, I just need to work out what part is the cause. CC @nikic to signal that we're doing something about this.

[0] http://llvm-compile-time-tracker.com/compare.php?from=c95253b1bac865b6d90cce186b7d665de163d50c&to=7d77bbef4ad9230f6f427649373fe46a668aa909&stat=instructions%3Au

[nikic]

FYI this has also increased time to build clang by a whopping 3%. My first suspicion is that your DebugProgramInstruction.h include in BasicBlock.h pulls in the kitchen sink.

[jmorse]

In D154080#4656037, @nikic wrote:

FYI this has also increased time to build clang by a whopping 3%. My first suspicion is that your DebugProgramInstruction.h include in BasicBlock.h pulls in the kitchen sink.

Ah, that's not on our radar at all, there'll be much to improve there.

[jmorse]

It looks like the major parts of the compile-time regression was down to the Metadata-handling changes in D153990, specifically handling DIArgList differently to other MDNodes causes a big IR-switch statement to not be optimised away, and that has a knock-on effect on inlining decisions all around Metadata.cpp. This is unfortunate, but it's also subject to change, as we're planning on re-profiling and then reimplementing how metadata-tracking works at a later date, once we're confident there are no more invasive changes. I've had some close chats with @StephenTozer and he's got some ideas on what we could change in the future to avoid an eventual regression.

Thus, in the meantime, I'm going to reland with the small key segments #ifdef'd away with the EXPERIMENTAL_DEBUGINFO_ITERATORS flag. This is enabled on this buildbot: https://lab.llvm.org/buildbot/#/builders/275 so it's not like we're landing dead code that will rot immediately.

The net result: DIArgList's are going to continue to be distinct without the compile-time flag on, which is alright because there can only ever be one reference to them, baked into a MetadataAsValue object that can be directly looked up out of LLVMContext (which is how things currently work). With the compile-time flag on, they'll be unique'd, and will require replaceable references to them to be maintained, because DebugValueUser's (aka DPValues) can refer to them.

(Compile-of-clang-time regression fixed by switching to forward declarations).

llvm/include/llvm/IR/BasicBlock.h

 #define LLVM_IR_BASICBLOCK_H
 
 #include "llvm-c/Types.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/ADT/ilist.h"
 #include "llvm/ADT/ilist_node.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/ADT/iterator_range.h"
+#include "llvm/IR/DebugProgramInstruction.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/SymbolTableListTraits.h"
 #include "llvm/IR/Value.h"
 #include <cassert>
 #include <cstddef>
 #include <iterator>
 
 namespace llvm {

 /// the blocks. The BasicBlock class allows malformed basic blocks to occur
 /// because it may be useful in the intermediate stage of constructing or
 /// modifying a program. However, the verifier will ensure that basic blocks are
 /// "well formed".
 class BasicBlock final : public Value, // Basic blocks are data objects also
                          public ilist_node_with_parent<BasicBlock, Function> {
 public:
   using InstListType = SymbolTableList<Instruction, ilist_iterator_bits<true>>;
+  /// Flag recording whether or not this block stores debug-info in the form
+  /// of intrinsic instructions (false) or non-instruction records (true).
+  bool IsNewDbgInfoFormat;
 
 private:
   friend class BlockAddress;
   friend class SymbolTableListTraits<BasicBlock>;
 
   InstListType InstList;
   Function *Parent;
 
+public:
+  /// Attach a DPMarker to the given instruction. Enables the storage of any
+  /// debug-info at this position in the program.
+  DPMarker *createMarker(Instruction *I);
+  DPMarker *createMarker(InstListType::iterator It);
+
+  /// Convert variable location debugging information stored in dbg.value
+  /// intrinsics into DPMarker / DPValue records. Deletes all dbg.values in
+  /// the process and sets IsNewDbgInfoFormat = true. Only takes effect if
+  /// the UseNewDbgInfoFormat LLVM command line option is given.
+  void convertToNewDbgValues();
+
+  /// Convert variable location debugging information stored in DPMarkers and
+  /// DPValues into the dbg.value intrinsic representation. Sets
+  /// IsNewDbgInfoFormat = false.
+  void convertFromNewDbgValues();
+
+  /// Ensure the block is in "old" dbg.value format (\p NewFlag == false) or
+  /// in the new format (\p NewFlag == true), converting to the desired format
+  /// if necessary.
+  void setIsNewDbgInfoFormat(bool NewFlag);
+
+  /// Validate any DPMarkers / DPValues attached to instructions in this block,
+  /// and block-level stored data too (TrailingDPValues).
+  /// \p Assert Should this method fire an assertion if a problem is found?
+  /// \p Msg Should this method print a message to errs() if a problem is found?
+  /// \p OS Output stream to write errors to.
+  /// \returns True if a problem is found.
+  bool validateDbgValues(bool Assert = true, bool Msg = false,
+                         raw_ostream *OS = nullptr);
+
+  /// Record that the collection of DPValues in \p M "trails" after the last
+  /// instruction of this block. These are equivalent to dbg.value intrinsics
+  /// that exist at the end of a basic block with no terminator (a transient
+  /// state that occurs regularly).
+  void setTrailingDPValues(DPMarker *M);
+
+  /// Fetch the collection of DPValues that "trail" after the last instruction
+  /// of this block, see \ref setTrailingDPValues. If there are none, returns
+  /// nullptr.
+  DPMarker *getTrailingDPValues();
+
+  /// Delete any trailing DPValues at the end of this block, see
+  /// \ref setTrailingDPValues.
+  void deleteTrailingDPValues();
+
+  void dumpDbgValues() const;
+
+private:
   void setParent(Function *parent);
 
   /// Constructor.
   ///
   /// If the function parameter is specified, the basic block is automatically
   /// inserted at either the end of the function (if InsertBefore is null), or
   /// before the specified basic block.
   explicit BasicBlock(LLVMContext &C, const Twine &Name = "",

llvm/include/llvm/IR/Function.h

private:
   /// Bits from GlobalObject::GlobalObjectSubclassData.
   enum {
     /// Whether this function is materializable.
     IsMaterializableBit = 0,
   };
 
   friend class SymbolTableListTraits<Function>;
 
+public:
+  /// Is this function using intrinsics to record the position of debugging
+  /// information, or non-intrinsic records? See IsNewDbgInfoFormat in
+  /// \ref BasicBlock.
+  bool IsNewDbgInfoFormat;
+
   /// hasLazyArguments/CheckLazyArguments - The argument list of a function is
   /// built on demand, so that the list isn't allocated until the first client
   /// needs it.  The hasLazyArguments predicate returns true if the arg list
   /// hasn't been set up yet.
-public:
   bool hasLazyArguments() const {
     return getSubclassDataFromValue() & (1<<0);
   }
 
+  /// \see BasicBlock::convertToNewDbgValues.
+  void convertToNewDbgValues();
+
+  /// \see BasicBlock::convertFromNewDbgValues.
+  void convertFromNewDbgValues();
+
+  void setIsNewDbgInfoFormat(bool NewVal);
+
 private:
   void CheckLazyArguments() const {
     if (hasLazyArguments())
       BuildLazyArguments();
   }
 
   void BuildLazyArguments() const;
 
public:
   ///
   /// Drop this function's arguments and splice in the ones from \c Src.
   /// Requires that this has no function body.
   void stealArgumentListFrom(Function &Src);
 
   /// Insert \p BB in the basic block list at \p Position. \Returns an iterator
   /// to the newly inserted BB.
   Function::iterator insert(Function::iterator Position, BasicBlock *BB) {
+    BB->setIsNewDbgInfoFormat(IsNewDbgInfoFormat);
     return BasicBlocks.insert(Position, BB);
   }
 
   /// Transfer all blocks from \p FromF to this function at \p ToIt.
   void splice(Function::iterator ToIt, Function *FromF) {
     splice(ToIt, FromF, FromF->begin(), FromF->end());
   }
 

llvm/include/llvm/IR/Module.h

DenseMap<std::pair<Intrinsic::ID, const FunctionType *>, unsigned>
                              ///< is used to make the intrinsic name unique.
 
   friend class Constant;
 
 /// @}
 /// @name Constructors
 /// @{
 public:
+  /// Is this Module using intrinsics to record the position of debugging
+  /// information, or non-intrinsic records? See IsNewDbgInfoFormat in
+  /// \ref BasicBlock.
+  bool IsNewDbgInfoFormat;
+
+  /// \see BasicBlock::convertToNewDbgValues.
+  void convertToNewDbgValues() {
+    for (auto &F : *this) {
+      F.convertToNewDbgValues();
+    }
+    IsNewDbgInfoFormat = true;
+  }
+
+  /// \see BasicBlock::convertFromNewDbgValues.
+  void convertFromNewDbgValues() {
+    for (auto &F : *this) {
+      F.convertFromNewDbgValues();
+    }
+    IsNewDbgInfoFormat = false;
+  }
+
   /// The Module constructor. Note that there is no default constructor. You
   /// must provide a name for the module upon construction.
   explicit Module(StringRef ModuleID, LLVMContext& C);
   /// The module destructor. This will dropAllReferences.
   ~Module();
 
 /// @}
 /// @name Module Level Accessors

llvm/lib/IR/BasicBlock.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Type.h"
+#include "llvm/Support/CommandLine.h"
+
+#include "LLVMContextImpl.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "ir"
 STATISTIC(NumInstrRenumberings, "Number of renumberings across all blocks");
 
+cl::opt<bool>
+    UseNewDbgInfoFormat("experimental-debuginfo-iterators",
+                        cl::desc("Enable communicating debuginfo positions "
+                                 "through iterators, eliminating intrinsics"),
+                        cl::init(false));
+
+DPMarker *BasicBlock::createMarker(Instruction *I) {
+  assert(IsNewDbgInfoFormat &&
+         "Tried to create a marker in a non new debug-info block!");
+  assert(I->DbgMarker == nullptr &&
+         "Tried to create marker for instuction that already has one!");
+  DPMarker *Marker = new DPMarker();
+  Marker->MarkedInstr = I;
+  I->DbgMarker = Marker;
+  return Marker;
+}
+
+DPMarker *BasicBlock::createMarker(InstListType::iterator It) {
+  assert(IsNewDbgInfoFormat &&
+         "Tried to create a marker in a non new debug-info block!");
+  if (It != end())
+    return createMarker(&*It);
+  DPMarker *DPM = getTrailingDPValues();
+  if (DPM)
+    return DPM;
+  DPM = new DPMarker();
+  setTrailingDPValues(DPM);
+  return DPM;
+}
+
+void BasicBlock::convertToNewDbgValues() {
+  // Is the command line option set?
+  if (!UseNewDbgInfoFormat)
+    return;
+
+  IsNewDbgInfoFormat = true;
+
+  // Iterate over all instructions in the instruction list, collecting dbg.value
+  // instructions and converting them to DPValues. Once we find a "real"
+  // instruction, attach all those DPValues to a DPMarker in that instruction.
+  SmallVector<DPValue *, 4> DPVals;
+  for (Instruction &I : make_early_inc_range(InstList)) {
+    assert(!I.DbgMarker && "DbgMarker already set on old-format instrs?");
+    if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I)) {
+      // Convert this dbg.value to a DPValue.
+      DPValue *Value = new DPValue(DVI);
+      DPVals.push_back(Value);
+      DVI->eraseFromParent();
+      continue;
+    }
+
+    // Create a marker to store DPValues in. Technically we don't need to store
+    // one marker per instruction, but that's a future optimisation.
+    createMarker(&I);
+    DPMarker *Marker = I.DbgMarker;
+
+    for (DPValue *DPV : DPVals)
+      Marker->insertDPValue(DPV, false);
+
+    DPVals.clear();
+  }
+}
+
+void BasicBlock::convertFromNewDbgValues() {
+  invalidateOrders();
+  IsNewDbgInfoFormat = false;
+
+  // Iterate over the block, finding instructions annotated with DPMarkers.
+  // Convert any attached DPValues to dbg.values and insert ahead of the
+  // instruction.
+  for (auto &Inst : *this) {
+    if (!Inst.DbgMarker)
+      continue;
+
+    DPMarker &Marker = *Inst.DbgMarker;
+    for (DPValue &DPV : Marker.getDbgValueRange())
+      InstList.insert(Inst.getIterator(),
+                      DPV.createDebugIntrinsic(getModule(), nullptr));
+
+    Marker.eraseFromParent();
+  };
+
+  // Assume no trailing DPValues: we could technically create them at the end
+  // of the block, after a terminator, but this would be non-cannonical and
+  // indicates that something else is broken somewhere.
+  assert(!getTrailingDPValues());
+}
+
+bool BasicBlock::validateDbgValues(bool Assert, bool Msg, raw_ostream *OS) {
+  bool RetVal = false;
+  if (!OS)
+    OS = &errs();
+
+  // Helper lambda for reporting failures: via assertion, printing, and return
+  // value.
+  auto TestFailure = [Assert, Msg, &RetVal, OS](bool Val, const char *Text) {
+    // Did the test fail?
+    if (Val)
+      return;
+
+    // If we're asserting, then fire off an assertion.
+    if (Assert)
+      llvm_unreachable(Text);
+
+    if (Msg)
+      *OS << Text << "\n";
+    RetVal = true;
+  };
+
+  // We should have the same debug-format as the parent function.
+  TestFailure(getParent()->IsNewDbgInfoFormat == IsNewDbgInfoFormat,
+              "Parent function doesn't have the same debug-info format");
+
+  // Only validate if we are using the new format.
+  if (!IsNewDbgInfoFormat)
+    return RetVal;
+
+  // Match every DPMarker to every Instruction and vice versa, and
+  // verify that there are no invalid DPValues.
+  for (auto It = begin(); It != end(); ++It) {
+    if (!It->DbgMarker)
+      continue;
+
+    // Validate DebugProgramMarkers.
+    DPMarker *CurrentDebugMarker = It->DbgMarker;
+
+    // If this is a marker, it should match the instruction and vice versa.
+    TestFailure(CurrentDebugMarker->MarkedInstr == &*It,
+                "Debug Marker points to incorrect instruction?");
+
+    // Now validate any DPValues in the marker.
+    for (DPValue &DPV : CurrentDebugMarker->getDbgValueRange()) {
+      // Validate DebugProgramValues.
+      TestFailure(DPV.getMarker() == CurrentDebugMarker,
+                  "Not pointing at correct next marker!");
+
+      // Verify that no DbgValues appear prior to PHIs.
+      TestFailure(
+          !isa<PHINode>(It),
+          "DebugProgramValues must not appear before PHI nodes in a block!");
+    }
+  }
+
+  // Except transiently when removing + re-inserting the block terminator, there
+  // should be no trailing DPValues.
+  TestFailure(!getTrailingDPValues(), "Trailing DPValues in block");
+  return RetVal;
+}
+
+void BasicBlock::setIsNewDbgInfoFormat(bool NewFlag) {
+  if (NewFlag && !IsNewDbgInfoFormat)
+    convertToNewDbgValues();
+  else if (!NewFlag && IsNewDbgInfoFormat)
+    convertFromNewDbgValues();
+}
+
 ValueSymbolTable *BasicBlock::getValueSymbolTable() {
   if (Function *F = getParent())
     return F->getValueSymbolTable();
   return nullptr;
 }
 
 LLVMContext &BasicBlock::getContext() const {
   return getType()->getContext();
 }
 
 template <> void llvm::invalidateParentIListOrdering(BasicBlock *BB) {
   BB->invalidateOrders();
 }
 
 // Explicit instantiation of SymbolTableListTraits since some of the methods
 // are not in the public header file...
 template class llvm::SymbolTableListTraits<Instruction,
                                            ilist_iterator_bits<true>>;
 
 BasicBlock::BasicBlock(LLVMContext &C, const Twine &Name, Function *NewParent,
                        BasicBlock *InsertBefore)
-  : Value(Type::getLabelTy(C), Value::BasicBlockVal), Parent(nullptr) {
+    : Value(Type::getLabelTy(C), Value::BasicBlockVal),
+      IsNewDbgInfoFormat(false), Parent(nullptr) {
 
   if (NewParent)
     insertInto(NewParent, InsertBefore);
   else
     assert(!InsertBefore &&
            "Cannot insert block before another block with no function!");
 
   setName(Name);
+  if (NewParent)
+    setIsNewDbgInfoFormat(NewParent->IsNewDbgInfoFormat);
 }
 
 void BasicBlock::insertInto(Function *NewParent, BasicBlock *InsertBefore) {
   assert(NewParent && "Expected a parent");
   assert(!Parent && "Already has a parent");
 
+  setIsNewDbgInfoFormat(NewParent->IsNewDbgInfoFormat);
+
   if (InsertBefore)
     NewParent->insert(InsertBefore->getIterator(), this);
   else
     NewParent->insert(NewParent->end(), this);
 }
 
 BasicBlock::~BasicBlock() {
   validateInstrOrdering();
while (!use_empty()) {
       BA->replaceAllUsesWith(ConstantExpr::getIntToPtr(Replacement,
                                                        BA->getType()));
       BA->destroyConstant();
     }
   }
 
   assert(getParent() == nullptr && "BasicBlock still linked into the program!");
   dropAllReferences();
+  for (auto &Inst : *this) {
+    if (!Inst.DbgMarker)
+      continue;
+    Inst.DbgMarker->eraseFromParent();
+  }
   InstList.clear();
 }
 
 void BasicBlock::setParent(Function *parent) {
   // Set Parent=parent, updating instruction symtab entries as appropriate.
   InstList.setSymTabObject(&Parent, parent);
 }
 
void BasicBlock::validateInstrOrdering() const {
   const Instruction *Prev = nullptr;
   for (const Instruction &I : *this) {
     assert((!Prev || Prev->comesBefore(&I)) &&
            "cached instruction ordering is incorrect");
     Prev = &I;
   }
 }
 #endif
+
+void BasicBlock::setTrailingDPValues(DPMarker *foo) {
+  getContext().pImpl->setTrailingDPValues(this, foo);
+}
+
+DPMarker *BasicBlock::getTrailingDPValues() {
+  return getContext().pImpl->getTrailingDPValues(this);
+}
+
+void BasicBlock::deleteTrailingDPValues() {
+  getContext().pImpl->deleteTrailingDPValues(this);
+}
+

llvm/lib/IR/DebugProgramInstruction.cpp

if (StoredDPValues.empty()) {
     Owner->DbgMarker = nullptr;
     return;
   }
 
   // The attached DPValues need to be preserved; attach them to the next
   // instruction. If there isn't a next instruction, put them on the
   // "trailing" list.
   // (This logic gets refactored in a future patch, needed to break some
-  //  dependencies here, and the TrailingDPValues facility only turns
-  //  up in the next patch).
+  //  dependencies here).
   BasicBlock::iterator NextInst = std::next(Owner->getIterator());
   DPMarker *NextMarker;
   if (NextInst == Owner->getParent()->end()) {
-    llvm_unreachable("Unimplemented trailing DPValue bit of RemoveDIs");
+    NextMarker = new DPMarker();
+    Owner->getParent()->setTrailingDPValues(NextMarker);
   } else {
     NextMarker = NextInst->DbgMarker;
   }
   NextMarker->absorbDebugValues(*this, true);
 
   eraseFromParent();
 }
 

llvm/lib/IR/Function.cpp

 // Explicit instantiations of SymbolTableListTraits since some of the methods
 // are not in the public header file...
 template class llvm::SymbolTableListTraits<BasicBlock>;
 
 static cl::opt<unsigned> NonGlobalValueMaxNameSize(
     "non-global-value-max-name-size", cl::Hidden, cl::init(1024),
     cl::desc("Maximum size for the name of non-global values."));
 
+void Function::convertToNewDbgValues() {
+  IsNewDbgInfoFormat = true;
+  for (auto &BB : *this) {
+    BB.convertToNewDbgValues();
+  }
+}
+
+void Function::convertFromNewDbgValues() {
+  IsNewDbgInfoFormat = false;
+  for (auto &BB : *this) {
+    BB.convertFromNewDbgValues();
+  }
+}
+
+void Function::setIsNewDbgInfoFormat(bool NewFlag) {
+  if (NewFlag && !IsNewDbgInfoFormat)
+    convertToNewDbgValues();
+  else if (!NewFlag && IsNewDbgInfoFormat)
+    convertFromNewDbgValues();
+}
+
 //===----------------------------------------------------------------------===//
 // Argument Implementation
 //===----------------------------------------------------------------------===//
 
 Argument::Argument(Type *Ty, const Twine &Name, Function *Par, unsigned ArgNo)
     : Value(Ty, Value::ArgumentVal), Parent(Par), ArgNo(ArgNo) {
   setName(Name);
 }
static unsigned computeAddrSpace(unsigned AddrSpace, Module *M) {
   return AddrSpace;
 }
 
 Function::Function(FunctionType *Ty, LinkageTypes Linkage, unsigned AddrSpace,
                    const Twine &name, Module *ParentModule)
     : GlobalObject(Ty, Value::FunctionVal,
                    OperandTraits<Function>::op_begin(this), 0, Linkage, name,
                    computeAddrSpace(AddrSpace, ParentModule)),
-      NumArgs(Ty->getNumParams()) {
+      NumArgs(Ty->getNumParams()), IsNewDbgInfoFormat(false) {
   assert(FunctionType::isValidReturnType(getReturnType()) &&
          "invalid return type");
   setGlobalObjectSubClassData(0);
 
   // We only need a symbol table for a function if the context keeps value names
   if (!getContext().shouldDiscardValueNames())
     SymTab = std::make_unique<ValueSymbolTable>(NonGlobalValueMaxNameSize);
 

llvm/lib/IR/Module.cpp

 template class llvm::SymbolTableListTraits<GlobalIFunc>;
 
 //===----------------------------------------------------------------------===//
 // Primitive Module methods.
 //
 
 Module::Module(StringRef MID, LLVMContext &C)
     : Context(C), ValSymTab(std::make_unique<ValueSymbolTable>(-1)),
-      ModuleID(std::string(MID)), SourceFileName(std::string(MID)), DL("") {
+      ModuleID(std::string(MID)), SourceFileName(std::string(MID)), DL(""),
+      IsNewDbgInfoFormat(false) {
   Context.addModule(this);
 }
 
 Module::~Module() {
   Context.removeModule(this);
   dropAllReferences();
   GlobalList.clear();
   FunctionList.clear();

llvm/lib/IR/Verifier.cpp

if (isa<PHINode>(BB.front())) {
     }
   }
 
   // Check that all instructions have their parent pointers set up correctly.
   for (auto &I : BB)
   {
     Check(I.getParent() == &BB, "Instruction has bogus parent pointer!");
   }
+
+  // Confirm that no issues arise from the debug program.
+  if (BB.IsNewDbgInfoFormat) {
+    // Configure the validate function to not fire assertions, instead print
+    // errors and return true if there's a problem.
+    bool RetVal = BB.validateDbgValues(false, true, OS);
+    Check(!RetVal, "Invalid configuration of new-debug-info data found");
+  }
 }
 
 void Verifier::visitTerminator(Instruction &I) {
   // Ensure that terminators only exist at the end of the basic block.
   Check(&I == I.getParent()->getTerminator(),
         "Terminator found in the middle of a basic block!", I.getParent());
   visitInstruction(I);
 }

llvm/lib/Linker/IRMover.cpp

Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) {
   GlobalValue *New = dyn_cast<GlobalValue>(*NewProto);
   if (!New)
     return *NewProto;
 
   // If we already created the body, just return.
   if (auto *F = dyn_cast<Function>(New)) {
     if (!F->isDeclaration())
       return New;
+    Function *OldFunc = cast<Function>(V);
+    F->IsNewDbgInfoFormat = OldFunc->IsNewDbgInfoFormat;
   } else if (auto *V = dyn_cast<GlobalVariable>(New)) {
     if (V->hasInitializer() || V->hasAppendingLinkage())
       return New;
   } else if (auto *GA = dyn_cast<GlobalAlias>(New)) {
     if (GA->getAliasee())
       return New;
   } else if (auto *GI = dyn_cast<GlobalIFunc>(New)) {
     if (GI->getResolver())
Error IRLinker::linkFunctionBody(Function &Dst, Function &Src) {
 
   // Link in the operands without remapping.
   if (Src.hasPrefixData())
     Dst.setPrefixData(Src.getPrefixData());
   if (Src.hasPrologueData())
     Dst.setPrologueData(Src.getPrologueData());
   if (Src.hasPersonalityFn())
     Dst.setPersonalityFn(Src.getPersonalityFn());
+  assert(Src.IsNewDbgInfoFormat == Dst.IsNewDbgInfoFormat);
 
   // Copy over the metadata attachments without remapping.
   Dst.copyMetadata(&Src, 0);
 
   // Steal arguments and splice the body of Src into Dst.
   Dst.stealArgumentListFrom(Src);
   Dst.splice(Dst.end(), &Src);
 

 }
 
 Error IRLinker::run() {
   // Ensure metadata materialized before value mapping.
   if (SrcM->getMaterializer())
     if (Error Err = SrcM->getMaterializer()->materializeMetadata())
       return Err;
 
+  DstM.IsNewDbgInfoFormat = SrcM->IsNewDbgInfoFormat;
+
   // Inherit the target data from the source module if the destination module
   // doesn't have one already.
   if (DstM.getDataLayout().isDefault())
     DstM.setDataLayout(SrcM->getDataLayout());
 
   // Copy the target triple from the source to dest if the dest's is empty.
   if (DstM.getTargetTriple().empty() && !SrcM->getTargetTriple().empty())
     DstM.setTargetTriple(SrcM->getTargetTriple());

llvm/lib/Target/AMDGPU/AMDGPURewriteOutArguments.cpp

bool AMDGPURewriteOutArguments::runOnFunction(Function &F) {
 
   AttributeMask RetAttrs;
   RetAttrs.addAttribute(Attribute::SExt);
   RetAttrs.addAttribute(Attribute::ZExt);
   RetAttrs.addAttribute(Attribute::NoAlias);
   NewFunc->removeRetAttrs(RetAttrs);
   // TODO: How to preserve metadata?
 
+  NewFunc->setIsNewDbgInfoFormat(F.IsNewDbgInfoFormat);
+
   // Move the body of the function into the new rewritten function, and replace
   // this function with a stub.
   NewFunc->splice(NewFunc->begin(), &F);
 
   for (std::pair<ReturnInst *, ReplacementVec> &Replacement : Replacements) {
     ReturnInst *RI = Replacement.first;
     IRBuilder<> B(RI);
     B.SetCurrentDebugLocation(RI->getDebugLoc());

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp

doPromotion(Function *F, FunctionAnalysisManager &FAM,
   // Construct the new function type using the new arguments.
   FunctionType *NFTy = FunctionType::get(RetTy, Params, FTy->isVarArg());
 
   // Create the new function body and insert it into the module.
   Function *NF = Function::Create(NFTy, F->getLinkage(), F->getAddressSpace(),
                                   F->getName());
   NF->copyAttributesFrom(F);
   NF->copyMetadata(F, 0);
+  NF->setIsNewDbgInfoFormat(F->IsNewDbgInfoFormat);
 
   // The new function will have the !dbg metadata copied from the original
   // function. The original function may not be deleted, and dbg metadata need
   // to be unique, so we need to drop it.
   F->setSubprogram(nullptr);
 
   LLVM_DEBUG(dbgs() << "ARG PROMOTION:  Promoting to:" << *NF << "\n"
                     << "From: " << *F);

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

bool DeadArgumentEliminationPass::deleteDeadVarargs(Function &F) {
   unsigned NumArgs = Params.size();
 
   // Create the new function body and insert it into the module...
   Function *NF = Function::Create(NFTy, F.getLinkage(), F.getAddressSpace());
   NF->copyAttributesFrom(&F);
   NF->setComdat(F.getComdat());
   F.getParent()->getFunctionList().insert(F.getIterator(), NF);
   NF->takeName(&F);
+  NF->IsNewDbgInfoFormat = F.IsNewDbgInfoFormat;
 
   // Loop over all the callers of the function, transforming the call sites
   // to pass in a smaller number of arguments into the new function.
   //
   std::vector<Value *> Args;
   for (User *U : llvm::make_early_inc_range(F.users())) {
     CallBase *CB = dyn_cast<CallBase>(U);
     if (!CB)
bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) {
   Function *NF = Function::Create(NFTy, F->getLinkage(), F->getAddressSpace());
   NF->copyAttributesFrom(F);
   NF->setComdat(F->getComdat());
   NF->setAttributes(NewPAL);
   // Insert the new function before the old function, so we won't be processing
   // it again.
   F->getParent()->getFunctionList().insert(F->getIterator(), NF);
   NF->takeName(F);
+  NF->IsNewDbgInfoFormat = F->IsNewDbgInfoFormat;
 
   // Loop over all the callers of the function, transforming the call sites to
   // pass in a smaller number of arguments into the new function.
   std::vector<Value *> Args;
   while (!F->use_empty()) {
     CallBase &CB = cast<CallBase>(*F->user_back());
 
     ArgAttrVec.clear();

llvm/lib/Transforms/Utils/CloneFunction.cpp

 #define DEBUG_TYPE "clone-function"
 
 /// See comments in Cloning.h.
 BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap,
                                   const Twine &NameSuffix, Function *F,
                                   ClonedCodeInfo *CodeInfo,
                                   DebugInfoFinder *DIFinder) {
   BasicBlock *NewBB = BasicBlock::Create(BB->getContext(), "", F);
+  NewBB->IsNewDbgInfoFormat = BB->IsNewDbgInfoFormat;
   if (BB->hasName())
     NewBB->setName(BB->getName() + NameSuffix);
 
   bool hasCalls = false, hasDynamicAllocas = false, hasMemProfMetadata = false;
   Module *TheModule = F ? F->getParent() : nullptr;
 
   // Loop over all instructions, and copy them over.
   for (const Instruction &I : *BB) {
void PruningFunctionCloner::CloneBlock(
   // Have we already cloned this block?
   if (BBEntry)
     return;
 
   // Nope, clone it now.
   BasicBlock *NewBB;
   Twine NewName(BB->hasName() ? Twine(BB->getName()) + NameSuffix : "");
   BBEntry = NewBB = BasicBlock::Create(BB->getContext(), NewName, NewFunc);
+  NewBB->IsNewDbgInfoFormat = BB->IsNewDbgInfoFormat;
 
   // It is only legal to clone a function if a block address within that
   // function is never referenced outside of the function.  Given that, we
   // want to map block addresses from the old function to block addresses in
   // the clone. (This is different from the generic ValueMapper
   // implementation, which generates an invalid blockaddress when
   // cloning a function.)
   //

llvm/unittests/IR/DebugInfoTest.cpp

 #include "llvm/IR/Module.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
+extern cl::opt<bool> UseNewDbgInfoFormat;
+
 static std::unique_ptr<Module> parseIR(LLVMContext &C, const char *IR) {
   SMDiagnostic Err;
   std::unique_ptr<Module> Mod = parseAssemblyString(IR, Err, C);
   if (!Mod)
     Err.print("DebugInfoTest", errs());
   return Mod;
 }
 
)");
   // These dbg.values are supposed to refer to different values.
   EXPECT_NE(DPV1->getRawLocation(), DPV2->getRawLocation());
 
   // Try manipulating DPValues and markers in the exit block.
   BasicBlock *ExitBlock = &*std::next(M->getFunction("f")->getEntryBlock().getIterator());
   Instruction *FirstInst = &ExitBlock->front();
   Instruction *RetInst = &*std::next(FirstInst->getIterator());
 
   // Set-up DPMarkers in this block.
-  FirstInst->DbgMarker = new DPMarker();
-  FirstInst->DbgMarker->MarkedInstr = FirstInst;
-  RetInst->DbgMarker = new DPMarker();
-  RetInst->DbgMarker->MarkedInstr = RetInst;
+  ExitBlock->IsNewDbgInfoFormat = true;
+  ExitBlock->createMarker(FirstInst);
+  ExitBlock->createMarker(RetInst);
 
   // Insert DPValues into markers, order should come out DPV2, DPV1.
   FirstInst->DbgMarker->insertDPValue(DPV1, false);
   FirstInst->DbgMarker->insertDPValue(DPV2, true);
   unsigned int ItCount = 0;
   for (DPValue &Item : FirstInst->DbgMarker->getDbgValueRange()) {
     EXPECT_TRUE((&Item == DPV2 && ItCount == 0) ||
               (&Item == DPV1 && ItCount == 1));
)");
   // basic block, then they should sink down into the "TrailingDPValues"
   // container for dangling debug-info. Future facilities will restore them
   // back when a terminator is inserted.
   FirstInst->DbgMarker->removeMarker();
   FirstInst->eraseFromParent();
   RetInst->DbgMarker->removeMarker();
   RetInst->eraseFromParent();
 
-  EXPECT_EQ(ExitBlock->TrailingDPValues.StoredDPValues.size(), 2);
+  DPMarker *EndMarker = ExitBlock->getTrailingDPValues();
+  ASSERT_NE(EndMarker, nullptr);
+  EXPECT_EQ(EndMarker->StoredDPValues.size(), 2);
   // Test again that it's those two DPValues, DPV1 and DPV2.
   ItCount = 0;
-  for (DPValue &Item : ExitBlock->TrailingDPValues.getDbgValueRange()) {
+  for (DPValue &Item : EndMarker->getDbgValueRange()) {
     EXPECT_TRUE((&Item == DPV2 && ItCount == 0) ||
               (&Item == DPV1 && ItCount == 1));
-    EXPECT_EQ(Item.getMarker(), &ExitBlock->TrailingDPValues);
+    EXPECT_EQ(Item.getMarker(), EndMarker);
     ++ItCount;
   }
+
+  // Those trailing DPValues would dangle and cause an assertion failure if
+  // they lived until the end of the LLVMContext,
+  ExitBlock->deleteTrailingDPValues();
+}
+
+TEST(MetadataTest, DPValueConversionRoutines) {
+  LLVMContext C;
+
+  // For the purpose of this test, set and un-set the command line option
+  // corresponding to UseNewDbgInfoFormat.
+  UseNewDbgInfoFormat = true;
+
+  std::unique_ptr<Module> M = parseIR(C, R"(
+    define i16 @f(i16 %a) !dbg !6 {
+      call void @llvm.dbg.value(metadata i16 %a, metadata !9, metadata !DIExpression()), !dbg !11
+      %b = add i16 %a, 1, !dbg !11
+      call void @llvm.dbg.value(metadata i16 %b, metadata !9, metadata !DIExpression()), !dbg !11
+      ret i16 0, !dbg !11
+
+    exit:
+      %c = add i16 %b, 1, !dbg !11
+      ret i16 0, !dbg !11
+    }
+    declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+    attributes #0 = { nounwind readnone speculatable willreturn }
+
+    !llvm.dbg.cu = !{!0}
+    !llvm.module.flags = !{!5}
+
+    !0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+    !1 = !DIFile(filename: "t.ll", directory: "/")
+    !2 = !{}
+    !5 = !{i32 2, !"Debug Info Version", i32 3}
+    !6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+    !7 = !DISubroutineType(types: !2)
+    !8 = !{!9}
+    !9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10)
+    !10 = !DIBasicType(name: "ty16", size: 16, encoding: DW_ATE_unsigned)
+    !11 = !DILocation(line: 1, column: 1, scope: !6)
+)");
+
+  // Check that the conversion routines and utilities between dbg.value
+  // debug-info format and DPValues works.
+  Function *F = M->getFunction("f");
+  BasicBlock *BB1 = &F->getEntryBlock();
+  // First instruction should be a dbg.value.
+  EXPECT_TRUE(isa<DbgValueInst>(BB1->front()));
+  EXPECT_FALSE(BB1->IsNewDbgInfoFormat);
+  // Validating the block for DPValues / DPMarkers shouldn't fail -- there's
+  // no data stored right now.
+  EXPECT_FALSE(BB1->validateDbgValues(false, false));
+
+  // Function and module should be marked as not having the new format too.
+  EXPECT_FALSE(F->IsNewDbgInfoFormat);
+  EXPECT_FALSE(M->IsNewDbgInfoFormat);
+
+  // Now convert.
+  M->convertToNewDbgValues();
+  EXPECT_TRUE(M->IsNewDbgInfoFormat);
+  EXPECT_TRUE(F->IsNewDbgInfoFormat);
+  EXPECT_TRUE(BB1->IsNewDbgInfoFormat);
+
+  // There should now be no dbg.value instructions!
+  // Ensure the first instruction exists, the test all of them.
+  EXPECT_FALSE(isa<DbgValueInst>(BB1->front()));
+  for (auto &BB : *F)
+    for (auto &I : BB)
+      EXPECT_FALSE(isa<DbgValueInst>(I));
+
+  // There should be a DPMarker on each of the two instructions in the entry
+  // block, each containing one DPValue.
+  EXPECT_EQ(BB1->size(), 2);
+  Instruction *FirstInst = &BB1->front();
+  Instruction *SecondInst = FirstInst->getNextNode();
+  ASSERT_TRUE(FirstInst->DbgMarker);
+  ASSERT_TRUE(SecondInst->DbgMarker);
+  EXPECT_NE(FirstInst->DbgMarker, SecondInst->DbgMarker);
+  EXPECT_EQ(FirstInst, FirstInst->DbgMarker->MarkedInstr);
+  EXPECT_EQ(SecondInst, SecondInst->DbgMarker->MarkedInstr);
+
+  EXPECT_EQ(FirstInst->DbgMarker->StoredDPValues.size(), 1);
+  DPValue *DPV1 = &*FirstInst->DbgMarker->getDbgValueRange().begin();
+  EXPECT_EQ(DPV1->getMarker(), FirstInst->DbgMarker);
+  // Should point at %a, an argument.
+  EXPECT_TRUE(isa<Argument>(DPV1->getVariableLocationOp(0)));
+
+  EXPECT_EQ(SecondInst->DbgMarker->StoredDPValues.size(), 1);
+  DPValue *DPV2 = &*SecondInst->DbgMarker->getDbgValueRange().begin();
+  EXPECT_EQ(DPV2->getMarker(), SecondInst->DbgMarker);
+  // Should point at FirstInst.
+  EXPECT_EQ(DPV2->getVariableLocationOp(0), FirstInst);
+
+  // There should be no DPValues / DPMarkers in the second block, but it should
+  // be marked as being in the new format.
+  BasicBlock *BB2 = BB1->getNextNode();
+  EXPECT_TRUE(BB2->IsNewDbgInfoFormat);
+  for (auto &Inst : *BB2)
+    // Either there should be no marker, or it should be empty.
+    EXPECT_TRUE(!Inst.DbgMarker || Inst.DbgMarker->StoredDPValues.empty());
+
+  // Validating the first block should continue to not be a problem,
+  EXPECT_FALSE(BB1->validateDbgValues(false, false));
+  // But if we were to break something, it should be able to fire. Don't attempt
+  // to comprehensively test the validator, it's a smoke-test rather than a
+  // "proper" verification pass.
+  DPV1->setMarker(nullptr);
+  // A marker pointing the wrong way should be an error.
+  EXPECT_TRUE(BB1->validateDbgValues(false, false));
+  DPV1->setMarker(FirstInst->DbgMarker);
+
+  DILocalVariable *DLV1 = DPV1->getVariable();
+  DIExpression *Expr1 = DPV1->getExpression();
+  DILocalVariable *DLV2 = DPV2->getVariable();
+  DIExpression *Expr2 = DPV2->getExpression();
+
+  // Convert everything back to the "old" format and ensure it's right.
+  M->convertFromNewDbgValues();
+  EXPECT_FALSE(M->IsNewDbgInfoFormat);
+  EXPECT_FALSE(F->IsNewDbgInfoFormat);
+  EXPECT_FALSE(BB1->IsNewDbgInfoFormat);
+
+  EXPECT_EQ(BB1->size(), 4);
+  ASSERT_TRUE(isa<DbgValueInst>(BB1->front()));
+  DbgValueInst *DVI1 = cast<DbgValueInst>(&BB1->front());
+  // These dbg.values should still point at the same places.
+  EXPECT_TRUE(isa<Argument>(DVI1->getVariableLocationOp(0)));
+  DbgValueInst *DVI2 = cast<DbgValueInst>(DVI1->getNextNode()->getNextNode());
+  EXPECT_EQ(DVI2->getVariableLocationOp(0), FirstInst);
+
+  // Check a few fields too,
+  EXPECT_EQ(DVI1->getVariable(), DLV1);
+  EXPECT_EQ(DVI1->getExpression(), Expr1);
+  EXPECT_EQ(DVI2->getVariable(), DLV2);
+  EXPECT_EQ(DVI2->getExpression(), Expr2);
+
+  UseNewDbgInfoFormat = false;
 }
 
 } // end namespace