[PATCH] D130315: [DWARF][BOLT] Implement new mechanism for DWARFRewriter

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added inline comments.


================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:55
+    uint32_t UnitOffset;
+    bool is_constructed = false;
+    uint32_t NewDieId = 0;
----------------



================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:58
+    std::unordered_map<uint64_t, uint32_t> DIEIDMap;
+    std::unordered_map<uint32_t, DIEInfo *> DIEId2Info;
+  };
----------------



================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:78
+  std::vector<std::unique_ptr<DIEAbbrev>> Abbreviations;
+  std::vector<DWARFUnit *> DWARF4TUList;
+  std::vector<DWARFUnit *> DWARF4CUList;
----------------



================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:79
+  std::vector<DWARFUnit *> DWARF4TUList;
+  std::vector<DWARFUnit *> DWARF4CUList;
+
----------------



================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:81
+
+  std::shared_timed_mutex AllocateDIEMutex;
+  BumpPtrAllocator DIEAlloc;
----------------
Is this necessary? I don't think this implementation is multi threaded. Right?


================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:108
+
+  /// CClone an attribute in reference format.
+  uint32_t cloneDieReferenceAttribute(
----------------



================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:124
+  /// output content.
+  void cloneExpression(DataExtractor &Data, DWARFExpression Expression,
+                       DWARFUnit &U, SmallVectorImpl<uint8_t> &OutputBuffer);
----------------
Should DWARFExpression be by reference?


================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:155
+  /// Update the Offset and Size of DIE.
+  uint32_t computeDIEOffset(DWARFUnit &CU, DIE &Die, uint32_t &curOffset);
+
----------------



================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:158
+  /// \return the unique ID of \p U if it exists.
+  Optional<uint32_t> getUnitId(DWARFUnit *DU) {
+    auto it = std::find(DUList.begin(), DUList.end(), DU);
----------------
Can DU be null? If not can you make it a const reference?
Tangental thought. Would it make sense to have a separate unordered map to look up of DWARFUnit pointer to uniqueID?
Instead of doing linear search every single time this function is invoked.
The uniqueID can be an index in this vector. 
So wrapper that ads *DU to this vector and stores size() -1 in the unordered map as ID, keyed by ptr address.
Something like that?


================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:172
+
+    llvm_unreachable("The DIE is not allocated before looking up it, some "
+                     "unexpected corner cases happened.");
----------------



================
Comment at: bolt/include/bolt/Rewrite/DIEBuilder.h:172
+
+    llvm_unreachable("The DIE is not allocated before looking up it, some "
+                     "unexpected corner cases happened.");
----------------
ayermolo wrote:
> 
Maybe just turn this in to print out of BOLT-ERROR?


================
Comment at: bolt/lib/Rewrite/DIEBuilder.cpp:37
+    DIEInfo *DstDIEInfo = ReferenceInfo->Dst;
+    UnitInfo &dstUnitInfo = getUnitInfo(DstDIEInfo->UnitId);
+    dwarf::Attribute Attr = ReferenceInfo->AttrSpec.Attr;
----------------



================
Comment at: bolt/lib/Rewrite/DIEBuilder.cpp:66
+
+void DIEBuilder::constructFromUnit(DWARFUnit *DU,
+                                   std::vector<DWARFUnit *> &DUOffsetList) {
----------------
If DU can't be null, pass by reference.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130315/new/

https://reviews.llvm.org/D130315