[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