[PATCH] D155604: [BOLT] Calculate input to output address map using BOLTLinker

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added a comment.

Apologies for the delay. Thanks for adding the map. I think we can eventually get rid of LocSyms, but we can do it in follow-up diffs.



================
Comment at: bolt/include/bolt/Core/AddressMap.h:1
+#ifndef BOLT_CORE_ADDRESS_MAP_H
+#define BOLT_CORE_ADDRESS_MAP_H
----------------
Needs a standard license header.


================
Comment at: bolt/include/bolt/Profile/BoltAddressTranslation.h:119
   void writeEntriesForBB(MapTy &Map, const BinaryBasicBlock &BB,
-                         uint64_t FuncAddress);
+                         uint64_t FuncAddress, const BinaryContext &BC);
 
----------------
Unneeded. Should be able to get `BC` via `BB`->`BF`->`BC`.


================
Comment at: bolt/include/bolt/Rewrite/RewriteInstance.h:384
+  /// in the output file.
+  static bool isLinkOnlySection(StringRef SectionName);
+
----------------
Maybe add a flag to `BinarySection`, instead? WDYT?


================
Comment at: bolt/lib/Core/AddressMap.cpp:45
+  Parsed.Map.reserve(Buffer.size() / 16);
+  const auto *const MapData = Buffer.data();
+
----------------
You can also use `DataExtractor` class for parsing and derive endianness from `BC.AsmInfo->isLittleEndian()`.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:3194
   emitBinaryContext(*Streamer, *BC, getOrgSecPrefix());
+  AddressMap::emit(*Streamer, *BC);
 
----------------
Can we make this call from inside `emitBinaryContext()`?


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:4032
 
+    if (isLinkOnlySection(Section.getName()))
+      continue;
----------------
Do we need this check and the one below despite calling `deregisterSection()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155604