[llvm] [nfc][InstrFDO]Encapsulate header writes in a class member function (PR #90142)
Mingming Liu via llvm-commits
llvm-commits at lists.llvm.org
Thu May 16 14:18:23 PDT 2024
minglotus-6 wrote:
> Instead of grouping the offsets of header fields into a struct, could we add a couple of member functions to `llvm::IndexedInstrProf::Header` instead?
>
> ```
> struct Header {
> // ... existing stuff ...
>
> uint64_t HeaderPos;
>
> // Write out the header, including placeholders for various offsets.
> void write(ProfOStream &OS, bool WritePrevVersion) {
> // Only write out the first four fields.
> for (int I = 0; I < 4; I++)
> OS.write(reinterpret_cast<uint64_t *>(&Header)[I]);
>
> UpdatePos = OS.tell();
> OS.write(0); // HashTableStart
> OS.write(0); // MemProfSectionStart
> OS.write(0); // BinaryIdSectionStart
> ...
> // Do whatever is appropriate with WritePrevVersion.
> }
>
> // Update the header.
> void patch(ProfOStream &OS, bool WritePrevVersion) {
> uint64_t Updates[] = {HashTableStart, MemProfSectionStart, BinaryIdSectionStart, ...};
> OS.patch({{UpdatePos, Updates, std::size(Updates)}});
> // Do whatever is appropriate with WritePrevVersion.
> }
> };
> ```
>
> This way, we can remove variables like `HashTableStartFieldOffset`, `MemProfSectionOffset`, and `BinaryIdSectionOffset`, which carry carry redundant information as a group. We know that `MemProfSectionOffset == HashTableStartFieldOffset + 8`, `BinaryIdSectionOffset == HashTableStartFieldOffset + 8`, etc. Also, the details of the header format are encapsulated in the new member functions, away from `InstrProfWriter`.
It makes a lot of sense to use compact information in the `*Offset` fields. In the same spirit as demonstrated in the comment, `writeHeader` now records `BackPatchStartOffset` as an output parameter.
`ProfOStream` (the output stream class) is defined in `InstrProfWriter.h/cpp`, and `IndexedInstrProf::Header` is defined in `InstrProf.h`. To make `writeHeader` a method of `Header` class, `ProfOStream` needs to be moved to `InstrProf.h` (or a common dependency of InstrProfWriter or InstrProf). So keep `writeHeader` a method of `InstrProfWriter` as discussed offline. PTAL, thanks!
https://github.com/llvm/llvm-project/pull/90142
More information about the llvm-commits
mailing list