[nfc][InstrFDO]Encapsulate header writes in a class member function

[minglotus-6]

The smaller class member are more focused and easier to maintain. This also paves the way for partial header forward compatibility in #88212

[snehasish]

Do you know why the size hint = 0?

I guess this is old code just being moved in this patch but we can drop the hint since now SmallVector can infer the size hint automatically.

[minglotus-6]

done.

[snehasish]

If this returned value isn't being used then maybe update the definition and call site? Also you can just return a HeaderFieldOffsets struct instead of passing it in by reference.

[minglotus-6]

Thanks for the review @snehasish !

Before making changes for the other comments, just a quick question on the return value: the subsequent patch for partial forward compatibility (#88212) needs header byte size (for reader to locate the end of the header fields), and InstrProfWriter::writeHeader is the function that has the header byte size information.

To make the diff smaller for the subsequent patch, I wonder if returning std::pair<uint64_t /* ByteSize*/, HeaderFieldOffsets> sounds good.

[snehasish]

Ah makes sense. I would suggest just leave a FIXME comment that the returned value will be used in a future patch at the call site.

[minglotus-6]

done by using a FIXME at the callsite.

Also chose (void) rather than std::ignore after I searched the differences and came across transmission/transmission#3667 (comment)

[snehasish]

Add a comment describing what this struct holds and how the contents are used.

[minglotus-6]

done.

[minglotus-6]

ptal, thanks!

[minglotus-6]

done.

[minglotus-6]

done.

[minglotus-6]

done by using a FIXME at the callsite.

Also chose (void) rather than std::ignore after I searched the differences and came across transmission/transmission#3667 (comment)

[kazutakahirata]

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.