[llvm] [nfc][InstrFDO]Encapsulate header writes in a class member function (PR #90142)

Kazu Hirata via llvm-commits llvm-commits at lists.llvm.org
Thu May 16 19:07:10 PDT 2024


================
@@ -763,16 +771,25 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
   }
   InfoObj->CSSummaryBuilder = nullptr;
 
+  const size_t MemProfOffset =
+      BackPatchStartOffset + sizeof(IndexedInstrProf::Header::HashOffset);
+  const size_t BinaryIdOffset =
+      MemProfOffset + sizeof(IndexedInstrProf::Header::MemProfOffset);
+  const size_t TemporalProfTracesOffset =
+      BinaryIdOffset + sizeof(IndexedInstrProf::Header::BinaryIdOffset);
+  const size_t VTableNamesOffset =
+      TemporalProfTracesOffset +
+      sizeof(IndexedInstrProf::Header::TemporalProfTracesOffset);
   if (!WritePrevVersion) {
     // Now do the final patch:
     PatchItem PatchItems[] = {
         // Patch the Header.HashOffset field.
-        {HashTableStartFieldOffset, &HashTableStart, 1},
+        {BackPatchStartOffset, &HashTableStart, 1},
----------------
kazutakahirata wrote:

May I suggest grouping these three variables?

```
uint64_t HeaderUpdates[] = {
  HashTableStart,
  MemProfSectionStart,
  BinaryIdSectionStart
};

PatchItem PatchItems[] = {
  {BackPatchStartOffset, HeaderUpdate, std::size(HeaderUpdates)},
  ...
```

I am suggesting this because:

- The essence of the header format is that these fields are written out one after another.  We don't really care about the offsets of these individual fields.
- Grouping them reduces the maintenance burden.  Adding a new section just involves adding another element to the `HeaderUpdates` array above.
- The `HeaderUpdates` array concisely describes the header format, at least the portion that needs patching.

Now, if you choose not to group them, I would write:

```
const size_t MemProfOffset = BackPatchStartOffset + sizeof(uint64_t);
```

because the size of each header field on disk comes from:

```
  void write(uint64_t V) { LE.write<uint64_t>(V); }
```

not the type of `IndexedInstrProf::Header::MemProfOffset`, etc.

https://github.com/llvm/llvm-project/pull/90142


More information about the llvm-commits mailing list