Addressed comments: DenseMap, pointer to reference, simplify if else

Author: grigorypas

bolt/include/bolt/Core/BinaryFunction.h

@@ -35,6 +35,7 @@
 #include "bolt/Core/JumpTable.h"
 #include "bolt/Core/MCPlus.h"
 #include "bolt/Utils/NameResolver.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
@@ -424,7 +425,8 @@ class BinaryFunction {
   unsigned LSDATypeEncoding{dwarf::DW_EH_PE_omit};
 
   /// All compilation units this function belongs to.
-  SmallVector<DWARFUnit *, 1> DwarfUnitVec;
+  /// Maps DWARF unit offset to the unit pointer.
+  DenseMap<uint64_t, DWARFUnit *> DwarfUnitMap;
 
   /// Last computed hash value. Note that the value could be recomputed using
   /// different parameters by every pass.
@@ -2409,19 +2411,16 @@ class BinaryFunction {
   void
   computeBlockHashes(HashFunction HashFunction = HashFunction::Default) const;
 
-  void addDWARFUnit(DWARFUnit *Unit) { DwarfUnitVec.push_back(Unit); }
+  void addDWARFUnit(DWARFUnit *Unit) { DwarfUnitMap[Unit->getOffset()] = Unit; }
 
   void removeDWARFUnit(DWARFUnit *Unit) {
-    auto *It = std::find(DwarfUnitVec.begin(), DwarfUnitVec.end(), Unit);
-    // If found, erase it
-    if (It != DwarfUnitVec.end()) {
-      DwarfUnitVec.erase(It);
-    }
+    DwarfUnitMap.erase(Unit->getOffset());
   }
 
   /// Return DWARF compile units for this function.
-  const SmallVector<DWARFUnit *, 1>& getDWARFUnits() const {
-    return DwarfUnitVec;
+  /// Returns a reference to the map of DWARF unit offsets to units.
+  const DenseMap<uint64_t, DWARFUnit *> &getDWARFUnits() const {
+    return DwarfUnitMap;
   }
 
   const DWARFDebugLine::LineTable *

bolt/include/bolt/Core/DebugData.h

@@ -867,7 +867,7 @@ class ClusteredRows {
   }
 
   template <typename T> void populate(const T Vec) {
-    assert(Vec.size() == Size && "");
+    assert(Vec.size() == Size && "Sizes must match");
     DebugLineTableRowRef *CurRawPtr = beginPtr();
     for (DebugLineTableRowRef RowRef : Vec) {
       *CurRawPtr = RowRef;

bolt/lib/Core/BinaryContext.cpp

@@ -1700,27 +1700,25 @@ void BinaryContext::preprocessDebugInfo() {
 
     // Go forward and add all units from ranges that cover the function
     while (++It != AllRanges.end()) {
-      if (It->LowPC <= FunctionAddress && FunctionAddress < It->HighPC) {
-        Function.addDWARFUnit(It->Unit);
-      } else {
+      if (It->LowPC > FunctionAddress || FunctionAddress >= It->HighPC)
         break;
-      }
+      Function.addDWARFUnit(It->Unit);
     }
   }
 
   // Discover units with debug info that needs to be updated.
   for (const auto &KV : BinaryFunctions) {
     const BinaryFunction &BF = KV.second;
     if (shouldEmit(BF) && !BF.getDWARFUnits().empty())
-      for (const DWARFUnit *Unit : BF.getDWARFUnits())
+      for (const auto &[_, Unit] : BF.getDWARFUnits())
         ProcessedCUs.insert(Unit);
   }
   // Clear debug info for functions from units that we are not going to process.
   for (auto &KV : BinaryFunctions) {
     BinaryFunction &BF = KV.second;
     // Collect units to remove to avoid iterator invalidation
     SmallVector<DWARFUnit *, 1> UnitsToRemove;
-    for (auto *Unit : BF.getDWARFUnits()) {
+    for (const auto &[_, Unit] : BF.getDWARFUnits()) {
       if (!ProcessedCUs.count(Unit))
         UnitsToRemove.push_back(Unit);
     }

bolt/lib/Core/BinaryEmitter.cpp

@@ -438,7 +438,7 @@ bool BinaryEmitter::emitFunction(BinaryFunction &Function,
   }
 
   if (opts::UpdateDebugSections && !Function.getDWARFUnits().empty())
-    for (DWARFUnit *Unit : Function.getDWARFUnits())
+    for (const auto &[_, Unit] : Function.getDWARFUnits())
       emitLineInfoEnd(Function, EndSymbol, Unit);
 
   // Exception handling info for the function.
@@ -687,10 +687,10 @@ SMLoc BinaryEmitter::emitLineInfo(const BinaryFunction &BF, SMLoc NewLoc,
   const ClusteredRows *Cluster = ClusteredRows::fromSMLoc(NewLoc);
 
   auto addToLineTable = [&](DebugLineTableRowRef RowReference,
-                            const DWARFUnit *TargetCU, unsigned Flags,
-                            MCSymbol *InstrLabel,
+                            const DWARFUnit &TargetCU, unsigned Flags,
+                            MCSymbol &InstrLabel,
                             const DWARFDebugLine::Row &CurrentRow) {
-    const uint64_t TargetUnitIndex = TargetCU->getOffset();
+    const uint64_t TargetUnitIndex = TargetCU.getOffset();
     unsigned TargetFilenum = CurrentRow.File;
     const uint32_t CurrentUnitIndex = RowReference.DwCompileUnitIndex;
     // If the CU id from the current instruction location does not
@@ -711,7 +711,7 @@ SMLoc BinaryEmitter::emitLineInfo(const BinaryFunction &BF, SMLoc NewLoc,
                                .getMCLineSections()
                                .getMCLineEntries();
     const auto *It = MapLineEntries.find(Streamer.getCurrentSectionOnly());
-    auto NewLineEntry = MCDwarfLineEntry(InstrLabel, DwarfLoc);
+    auto NewLineEntry = MCDwarfLineEntry(&InstrLabel, DwarfLoc);
 
     // Check if line table exists and has entries before doing comparison
     if (It != MapLineEntries.end() && !It->second.empty()) {
@@ -750,21 +750,17 @@ SMLoc BinaryEmitter::emitLineInfo(const BinaryFunction &BF, SMLoc NewLoc,
     if (FirstInstr)
       Flags |= DWARF2_FLAG_IS_STMT;
     const auto &FunctionDwarfUnits = BF.getDWARFUnits();
-    const auto *It = std::find_if(
-        FunctionDwarfUnits.begin(), FunctionDwarfUnits.end(),
-        [RowReference](const DWARFUnit *Unit) {
-          return Unit->getOffset() == RowReference.DwCompileUnitIndex;
-        });
+    auto It = FunctionDwarfUnits.find(RowReference.DwCompileUnitIndex);
     if (It != FunctionDwarfUnits.end()) {
-      addToLineTable(RowReference, *It, Flags, InstrLabel, CurrentRow);
+      addToLineTable(RowReference, *It->second, Flags, *InstrLabel, CurrentRow);
       continue;
     }
     // This rows is from CU that did not contain the original function.
     // This might happen if BOLT moved/inlined that instruction from other CUs.
     // In this case, we need to insert it to all CUs that the function
     // originally beloned to.
-    for (const DWARFUnit *Unit : BF.getDWARFUnits()) {
-      addToLineTable(RowReference, Unit, Flags, InstrLabel, CurrentRow);
+    for (const auto &[_, Unit] : BF.getDWARFUnits()) {
+      addToLineTable(RowReference, *Unit, Flags, *InstrLabel, CurrentRow);
     }
   }
 
@@ -1148,7 +1144,7 @@ void BinaryEmitter::emitDebugLineInfoForOriginalFunctions() {
       continue;
 
     // Loop through all CUs in the function
-    for (DWARFUnit *Unit : Function.getDWARFUnits()) {
+    for (const auto &[_, Unit] : Function.getDWARFUnits()) {
       const DWARFDebugLine::LineTable *LineTable =
           Function.getDWARFLineTableForUnit(Unit);
       if (!LineTable)

bolt/lib/Core/BinaryFunction.cpp

@@ -1490,7 +1490,7 @@ Error BinaryFunction::disassemble() {
 add_instruction:
     if (!getDWARFUnits().empty()) {
       SmallVector<DebugLineTableRowRef, 1> Rows;
-      for (DWARFUnit *Unit : getDWARFUnits()) {
+      for (const auto &[_, Unit] : getDWARFUnits()) {
         const DWARFDebugLine::LineTable *LineTable =
             getDWARFLineTableForUnit(Unit);
         if (!LineTable)