[DebugInfo] Make InstrRange into a class, NFC

[dstenb]

Summary

Replace use of std::pair by creating a class for the debug value
instruction ranges instead. This is a preparatory refactoring for
improving handling of clobbered fragments.

In an upcoming commit the Begin pointer will become a PointerIntPair, so
it will be cleaner to have a getter for that.

[aprantl]

LGTM with some small style nitpicks inline.

include/llvm/CodeGen/DbgEntityHistoryCalculator.h

33 ↗

(On Diff #192643)

class Doxygen comment please

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

50 ↗

(On Diff #192643)

!Ranges.back().getEnd() or introduce a helper such as Ranges.back().isOpen()

80 ↗

(On Diff #192643)

ditto

[dstenb]

Address review comments.

[dstenb]

include/llvm/CodeGen/DbgEntityHistoryCalculator.h

33 ↗

(On Diff #192643)

I took the comment that was listed above and doxygenized it. I made some adjustments, since the explanation about the range's end was not really correct for fragments.

(The class will change quite a bit in the follow-up patches, so the End explanation will not live for long, but I thought that it could still be good.)

[dstenb]

Slight adjustment: Add an isEnded() query function rather than using getEnd(). An isEnded() function is added in the follow-up patch D59941, so we can avoid unnecessary diffs in that patch by adding such a function here also.

[aprantl]

include/llvm/CodeGen/DbgEntityHistoryCalculator.h

45 ↗

(On Diff #194328)

I think the proper terminology for this may be isOpen / is Closed? https://en.wikipedia.org/wiki/Interval_(mathematics)

[dstenb]

include/llvm/CodeGen/DbgEntityHistoryCalculator.h

45 ↗

(On Diff #194328)

Sorry, yes! I'll change it to "Closed" before landing this.

llvm/trunk/include/llvm/CodeGen/DbgEntityHistoryCalculator.h

 class DILocalVariable;
 class MachineFunction;
 class MachineInstr;
 class TargetRegisterInfo;
 
 // For each user variable, keep a list of instruction ranges where this variable
 // is accessible. The variables are listed in order of appearance.
 class DbgValueHistoryMap {
-  // Each instruction range starts with a DBG_VALUE instruction, specifying the
-  // location of a variable, which is assumed to be valid until the end of the
-  // range. If end is not specified, location is valid until the start
-  // instruction of the next instruction range, or until the end of the
-  // function.
 public:
-  using InstrRange = std::pair<const MachineInstr *, const MachineInstr *>;
+  /// Specifies an instruction range where a DBG_VALUE is valid.
+  ///
+  /// \p Begin is a DBG_VALUE instruction, specifying the location of a
+  /// variable, which is assumed to be valid until the end of the range. If \p
+  /// End is not specified, the location is valid until the first overlapping
+  /// DBG_VALUE if any such DBG_VALUE exists, otherwise it is valid until the
+  /// end of the function.
+  class InstrRange {
+    const MachineInstr *Begin;
+    const MachineInstr *End;
+
+  public:
+    InstrRange(const MachineInstr *Begin) : Begin(Begin), End(nullptr) {}
+
+    const MachineInstr *getBegin() const { return Begin; }
+    const MachineInstr *getEnd() const { return End; }
+
+    bool isClosed() const { return End; }
+
+    void endRange(const MachineInstr &End);
+  };
   using InstrRanges = SmallVector<InstrRange, 4>;
   using InlinedEntity = std::pair<const DINode *, const DILocation *>;
   using InstrRangesMap = MapVector<InlinedEntity, InstrRanges>;
 
 private:
   InstrRangesMap VarInstrRanges;
 
 public:

llvm/trunk/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

 }
 
 void CodeViewDebug::calculateRanges(
     LocalVariable &Var, const DbgValueHistoryMap::InstrRanges &Ranges) {
   const TargetRegisterInfo *TRI = Asm->MF->getSubtarget().getRegisterInfo();
 
   // Calculate the definition ranges.
   for (auto I = Ranges.begin(), E = Ranges.end(); I != E; ++I) {
-    const InsnRange &Range = *I;
-    const MachineInstr *DVInst = Range.first;
+    const auto &Range = *I;
+    const MachineInstr *DVInst = Range.getBegin();
     assert(DVInst->isDebugValue() && "Invalid History entry");
     // FIXME: Find a way to represent constant variables, since they are
     // relatively common.
     Optional<DbgVariableLocation> Location =
         DbgVariableLocation::extractFromMachineInstruction(*DVInst);
     if (!Location)
       continue;
 
if (Location->Register == 0 || Location->LoadChain.size() > 1)
 
       if (Var.DefRanges.empty() ||
           Var.DefRanges.back().isDifferentLocation(DR)) {
         Var.DefRanges.emplace_back(std::move(DR));
       }
     }
 
     // Compute the label range.
-    const MCSymbol *Begin = getLabelBeforeInsn(Range.first);
-    const MCSymbol *End = getLabelAfterInsn(Range.second);
+    const MCSymbol *Begin = getLabelBeforeInsn(Range.getBegin());
+    const MCSymbol *End = getLabelAfterInsn(Range.getEnd());
     if (!End) {
       // This range is valid until the next overlapping bitpiece. In the
       // common case, ranges will not be bitpieces, so they will overlap.
       auto J = std::next(I);
       const DIExpression *DIExpr = DVInst->getDebugExpression();
       while (J != E &&
-             !DIExpr->fragmentsOverlap(J->first->getDebugExpression()))
+             !DIExpr->fragmentsOverlap(J->getBegin()->getDebugExpression()))
         ++J;
       if (J != E)
-        End = getLabelBeforeInsn(J->first);
+        End = getLabelBeforeInsn(J->getBegin());
       else
         End = Asm->getFunctionEnd();
     }
 
     // If the last range end is our begin, just extend the last range.
     // Otherwise make a new range.
     SmallVectorImpl<std::pair<const MCSymbol *, const MCSymbol *>> &R =
         Var.DefRanges.back().Ranges;

llvm/trunk/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

 }
 
 void DbgValueHistoryMap::startInstrRange(InlinedEntity Var,
                                          const MachineInstr &MI) {
   // Instruction range should start with a DBG_VALUE instruction for the
   // variable.
   assert(MI.isDebugValue() && "not a DBG_VALUE");
   auto &Ranges = VarInstrRanges[Var];
-  if (!Ranges.empty() && Ranges.back().second == nullptr &&
-      Ranges.back().first->isIdenticalTo(MI)) {
+  if (!Ranges.empty() && !Ranges.back().isClosed() &&
+      Ranges.back().getBegin()->isIdenticalTo(MI)) {
     LLVM_DEBUG(dbgs() << "Coalescing identical DBG_VALUE entries:\n"
-                      << "\t" << Ranges.back().first << "\t" << MI << "\n");
+                      << "\t" << Ranges.back().getBegin() << "\t" << MI
+                      << "\n");
     return;
   }
-  Ranges.push_back(std::make_pair(&MI, nullptr));
+  Ranges.emplace_back(&MI);
 }
 
 void DbgValueHistoryMap::endInstrRange(InlinedEntity Var,
                                        const MachineInstr &MI) {
   auto &Ranges = VarInstrRanges[Var];
-  // Verify that the current instruction range is not yet closed.
-  assert(!Ranges.empty() && Ranges.back().second == nullptr);
+  assert(!Ranges.empty() && "No range exists for variable!");
+  Ranges.back().endRange(MI);
+}
+
+void DbgValueHistoryMap::InstrRange::endRange(const MachineInstr &MI) {
   // For now, instruction ranges are not allowed to cross basic block
   // boundaries.
-  assert(Ranges.back().first->getParent() == MI.getParent());
-  Ranges.back().second = &MI;
+  assert(Begin->getParent() == MI.getParent());
+  assert(!isClosed() && "Range is already closed!");
+  End = &MI;
 }
 
 unsigned DbgValueHistoryMap::getRegisterForVar(InlinedEntity Var) const {
   const auto &I = VarInstrRanges.find(Var);
   if (I == VarInstrRanges.end())
     return 0;
   const auto &Ranges = I->second;
-  if (Ranges.empty() || Ranges.back().second != nullptr)
+  if (Ranges.empty() || Ranges.back().isClosed())
     return 0;
-  return isDescribedByReg(*Ranges.back().first);
+  return isDescribedByReg(*Ranges.back().getBegin());
 }
 
 void DbgLabelInstrMap::addInstr(InlinedEntity Label, const MachineInstr &MI) {
   assert(MI.isDebugLabel() && "not a DBG_LABEL");
   LabelInstr[Label] = &MI;
 }
 
 namespace {
if (Location)
       dbgs() << Location->getFilename() << ":" << Location->getLine() << ":"
              << Location->getColumn();
     else
       dbgs() << "<unknown location>";
 
     dbgs() << " --\n";
 
     for (const InstrRange &Range : Ranges) {
-      dbgs() << "   Begin: " << *Range.first;
-      if (Range.second)
-        dbgs() << "   End  : " << *Range.second;
+      dbgs() << "   Begin: " << *Range.getBegin();
+      if (Range.getEnd())
+        dbgs() << "   End  : " << *Range.getEnd();
       dbgs() << "\n";
     }
   }
 }
 #endif

llvm/trunk/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

for (const auto &I : DbgValues) {
     // so arguments are visible when breaking at function entry.
     //
     // We do not change the label for values that are described by registers,
     // as that could place them above their defining instructions. We should
     // ideally not change the labels for constant debug values either, since
     // doing that violates the ranges that are calculated in the history map.
     // However, we currently do not emit debug values for constant arguments
     // directly at the start of the function, so this code is still useful.
-    const DILocalVariable *DIVar = Ranges.front().first->getDebugVariable();
+    const DILocalVariable *DIVar =
+        Ranges.front().getBegin()->getDebugVariable();
     if (DIVar->isParameter() &&
         getDISubprogram(DIVar->getScope())->describes(&MF->getFunction())) {
-      if (!IsDescribedByReg(Ranges.front().first))
-        LabelsBeforeInsn[Ranges.front().first] = Asm->getFunctionBegin();
-      if (Ranges.front().first->getDebugExpression()->isFragment()) {
+      if (!IsDescribedByReg(Ranges.front().getBegin()))
+        LabelsBeforeInsn[Ranges.front().getBegin()] = Asm->getFunctionBegin();
+      if (Ranges.front().getBegin()->getDebugExpression()->isFragment()) {
         // Mark all non-overlapping initial fragments.
         for (auto I = Ranges.begin(); I != Ranges.end(); ++I) {
-          const DIExpression *Fragment = I->first->getDebugExpression();
+          const DIExpression *Fragment = I->getBegin()->getDebugExpression();
           if (std::any_of(Ranges.begin(), I,
                           [&](DbgValueHistoryMap::InstrRange Pred) {
                             return Fragment->fragmentsOverlap(
-                                Pred.first->getDebugExpression());
+                                Pred.getBegin()->getDebugExpression());
                           }))
             break;
-          if (!IsDescribedByReg(I->first))
-            LabelsBeforeInsn[I->first] = Asm->getFunctionBegin();
+          if (!IsDescribedByReg(I->getBegin()))
+            LabelsBeforeInsn[I->getBegin()] = Asm->getFunctionBegin();
         }
       }
     }
 
     for (const auto &Range : Ranges) {
-      requestLabelBeforeInsn(Range.first);
-      if (Range.second)
-        requestLabelAfterInsn(Range.second);
+      requestLabelBeforeInsn(Range.getBegin());
+      if (Range.getEnd())
+        requestLabelAfterInsn(Range.getEnd());
     }
   }
 
   // Ensure there is a symbol before DBG_LABEL.
   for (const auto &I : DbgLabels) {
     const MachineInstr *MI = I.second;
     requestLabelBeforeInsn(MI);
   }

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

 // [3-4]    [x, (reg1, fragment 32, 32)]
 // [4- ]    [x, (mem,  fragment  0, 64)]
 void
 DwarfDebug::buildLocationList(SmallVectorImpl<DebugLocEntry> &DebugLoc,
                               const DbgValueHistoryMap::InstrRanges &Ranges) {
   SmallVector<DebugLocEntry::Value, 4> OpenRanges;
 
   for (auto I = Ranges.begin(), E = Ranges.end(); I != E; ++I) {
-    const MachineInstr *Begin = I->first;
-    const MachineInstr *End = I->second;
+    const MachineInstr *Begin = I->getBegin();
+    const MachineInstr *End = I->getEnd();
     assert(Begin->isDebugValue() && "Invalid History entry");
 
     // Check if a variable is inaccessible in this range.
     if (Begin->getNumOperands() > 1 &&
         Begin->getOperand(0).isReg() && !Begin->getOperand(0).getReg()) {
       OpenRanges.clear();
       continue;
     }
for (auto I = Ranges.begin(), E = Ranges.end(); I != E; ++I) {
     assert(StartLabel && "Forgot label before DBG_VALUE starting a range!");
 
     const MCSymbol *EndLabel;
     if (End != nullptr)
       EndLabel = getLabelAfterInsn(End);
     else if (std::next(I) == Ranges.end())
       EndLabel = Asm->getFunctionEnd();
     else
-      EndLabel = getLabelBeforeInsn(std::next(I)->first);
+      EndLabel = getLabelBeforeInsn(std::next(I)->getBegin());
     assert(EndLabel && "Forgot label after instruction ending a range!");
 
     LLVM_DEBUG(dbgs() << "DotDebugLoc: " << *Begin << "\n");
 
     auto Value = getDebugLocValue(Begin);
     OpenRanges.push_back(Value);
 
     // Omit entries with empty ranges as they do not have any effect in DWARF.
for (const auto &I : DbgValues) {
     // If variable scope is not found then skip this variable.
     if (!Scope)
       continue;
 
     Processed.insert(IV);
     DbgVariable *RegVar = cast<DbgVariable>(createConcreteEntity(TheCU,
                                             *Scope, LocalVar, IV.second));
 
-    const MachineInstr *MInsn = Ranges.front().first;
+    const MachineInstr *MInsn = Ranges.front().getBegin();
     assert(MInsn->isDebugValue() && "History must begin with debug value");
 
     // Check if there is a single DBG_VALUE, valid throughout the var's scope.
     if (Ranges.size() == 1 &&
-        validThroughout(LScopes, MInsn, Ranges.front().second)) {
+        validThroughout(LScopes, MInsn, Ranges.front().getEnd())) {
       RegVar->initializeDbgValue(MInsn);
       continue;
     }
     // Do not emit location lists if .debug_loc secton is disabled.
     if (!useLocSection())
       continue;
 
     // Handle multiple DBG_VALUE instructions describing one variable.