[BOLT] Embed cold mapping info into function entry in BAT #76903
Conversation
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
| @@ -160,12 +164,31 @@ std::error_code BoltAddressTranslation::parse(StringRef Buf) { | |||
| return make_error_code(llvm::errc::io_error); | |||
|
|
|||
| Error Err(Error::success()); | |||
| std::vector<uint64_t> HotFuncs; | |||
There was a problem hiding this comment.
Is this HotFuncs vector going to be used in another diff of this stack? It looks unused to me atm.
There was a problem hiding this comment.
It's populated by parseMaps<Hot> and read by parseMaps<Cold>.
Cold functions encode indices of parent hot functions (also delta encoded), hence this vector is required.
| @@ -78,10 +79,20 @@ class BoltAddressTranslation { | |||
|
|
|||
| BoltAddressTranslation() {} | |||
|
|
|||
| /// Write the serialized address translation table for a function. | |||
| template <bool Cold> | |||
| /// Write the serialized address translation tables for each reordered | ||
| /// function | ||
| void write(const BinaryContext &BC, raw_ostream &OS); | ||
|
|
||
| /// Read the serialized address translation table for a function. | ||
| /// Return a parse error if failed. | ||
| template <bool Cold> |
| @@ -11,6 +11,7 @@ | |||
|
|
|||
| #include "llvm/ADT/SmallVector.h" | |||
| #include "llvm/ADT/StringRef.h" | |||
| #include "llvm/Support/DataExtractor.h" | |||
There was a problem hiding this comment.
Not sure why was this moved here
There was a problem hiding this comment.
DataExtractor type is used in parseMaps declaration
| @@ -119,6 +130,7 @@ class BoltAddressTranslation { | |||
| uint64_t FuncAddress); | |||
|
|
|||
| std::map<uint64_t, MapTy> Maps; | |||
| std::map<uint64_t, MapTy> ColdMaps; | |||
There was a problem hiding this comment.
Kind of weird that this is only used for write(), and not during parsing. There is a lost symmetry between write() and parse() after this diff. Maybe move ColdMaps to live inside write() method then, since it is never used outside of it.
Once we write(), we're not able to call translate(), since we lost cold functions in Maps. Not that this workflow was ever used, but if this class was meant to have any sort of "serialize/deserialize" workflow, now they are out of sync, and design-wise it feels off. So maybe we should use both a private "Maps" / "ColdMaps" during write(), so the code doesn't read as if "translate" depends on either write or parse -- it should depends only on parse. And maybe write() should be a stateless static method.
There was a problem hiding this comment.
Removed the use of ColdMaps.
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
|
@rafaelauler: addressed the feedback you've added. However, with template bool parameter ( |
aaupov
changed the base branch from
users/aaupov/spr/main.bolt-embed-cold-mapping-info-into-function-entry-in-bat
to
main
aaupov
deleted the
users/aaupov/spr/bolt-embed-cold-mapping-info-into-function-entry-in-bat
branch
Reduces BAT section size: - large binary: to 12283500 bytes (0.32x original size), - medium binary: to 1616020 bytes (0.27x original size), - small binary: to 404 bytes (0.28x original size). Test Plan: Updated bolt/test/X86/bolt-address-translation.test
Reduces BAT section size:
Test Plan: Updated bolt/test/X86/bolt-address-translation.test