Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
90109db
[nfc][InstrFDO]Encapsulate header writes in a class member function
mingmingl-llvm Apr 25, 2024
3c108ad
'void' unused return value. The value will be used in a follow-up patch
mingmingl-llvm Apr 25, 2024
db86e88
resolve review feedback
mingmingl-llvm Apr 30, 2024
8b6256d
resolve review feedback
mingmingl-llvm May 16, 2024
22d066a
clean up
mingmingl-llvm May 16, 2024
379a032
resolve review feedback
mingmingl-llvm May 17, 2024
464a717
Update llvm/include/llvm/ProfileData/InstrProfWriter.h
mingmingl-llvm May 17, 2024
b15bcc2
Update llvm/lib/ProfileData/InstrProfWriter.cpp
mingmingl-llvm May 17, 2024
1faf269
Update llvm/lib/ProfileData/InstrProfWriter.cpp
mingmingl-llvm May 17, 2024
a855e0a
update comment
mingmingl-llvm May 17, 2024
7b82574
remove unused field
mingmingl-llvm May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions llvm/include/llvm/ProfileData/InstrProfWriter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,12 @@ class InstrProfWriter {
void addTemporalProfileTrace(TemporalProfTraceTy Trace);

Error writeImpl(ProfOStream &OS);

// Writes known header fields and reserves space for fields whose value are
// known only after payloads are written. Returns the start byte offset for
// back patching.
uint64_t writeHeader(const IndexedInstrProf::Header &header,
const bool WritePrevVersion, ProfOStream &OS);
};

} // end namespace llvm
Expand Down
70 changes: 34 additions & 36 deletions llvm/lib/ProfileData/InstrProfWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,27 @@ static Error writeMemProf(
memprof::MaximumSupportedVersion));
}

uint64_t InstrProfWriter::writeHeader(const IndexedInstrProf::Header &Header,
const bool WritePrevVersion,
ProfOStream &OS) {
// Only write out the first four fields.
for (int I = 0; I < 4; I++)
OS.write(reinterpret_cast<const uint64_t *>(&Header)[I]);

// Remember the offset of the remaining fields to allow back patching later.
auto BackPatchStartOffset = OS.tell();

// Reserve the space for back patching later.
OS.write(0); // HashOffset
OS.write(0); // MemProfOffset
OS.write(0); // BinaryIdOffset
OS.write(0); // TemporalProfTracesOffset
if (!WritePrevVersion)
OS.write(0); // VTableNamesOffset

return BackPatchStartOffset;
}

Error InstrProfWriter::writeImpl(ProfOStream &OS) {
using namespace IndexedInstrProf;
using namespace support;
Expand All @@ -564,7 +585,7 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
InfoObj->CSSummaryBuilder = &CSISB;

// Populate the hash table generator.
SmallVector<std::pair<StringRef, const ProfilingData *>, 0> OrderedData;
SmallVector<std::pair<StringRef, const ProfilingData *>> OrderedData;
for (const auto &I : FunctionData)
if (shouldEncodeData(I.getValue()))
OrderedData.emplace_back((I.getKey()), &I.getValue());
Expand Down Expand Up @@ -606,35 +627,8 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
Header.TemporalProfTracesOffset = 0;
Header.VTableNamesOffset = 0;

// Only write out the first four fields. We need to remember the offset of the
// remaining fields to allow back patching later.
for (int I = 0; I < 4; I++)
OS.write(reinterpret_cast<uint64_t *>(&Header)[I]);

// Save the location of Header.HashOffset field in \c OS.
uint64_t HashTableStartFieldOffset = OS.tell();
// Reserve the space for HashOffset field.
OS.write(0);

// Save the location of MemProf profile data. This is stored in two parts as
// the schema and as a separate on-disk chained hashtable.
uint64_t MemProfSectionOffset = OS.tell();
// Reserve space for the MemProf table field to be patched later if this
// profile contains memory profile information.
OS.write(0);

// Save the location of binary ids section.
uint64_t BinaryIdSectionOffset = OS.tell();
// Reserve space for the BinaryIdOffset field to be patched later if this
// profile contains binary ids.
OS.write(0);

uint64_t TemporalProfTracesOffset = OS.tell();
OS.write(0);

uint64_t VTableNamesOffset = OS.tell();
if (!WritePrevVersion)
OS.write(0);
const uint64_t BackPatchStartOffset =
writeHeader(Header, WritePrevVersion, OS);

// Reserve space to write profile summary data.
uint32_t NumEntries = ProfileSummaryBuilder::DefaultCutoffs.size();
Expand Down Expand Up @@ -763,16 +757,20 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
}
InfoObj->CSSummaryBuilder = nullptr;

const size_t MemProfOffset = BackPatchStartOffset + sizeof(uint64_t);
const size_t BinaryIdOffset = MemProfOffset + sizeof(uint64_t);
const size_t TemporalProfTracesOffset = BinaryIdOffset + sizeof(uint64_t);
const size_t VTableNamesOffset = TemporalProfTracesOffset + sizeof(uint64_t);
if (!WritePrevVersion) {
// Now do the final patch:
PatchItem PatchItems[] = {
// Patch the Header.HashOffset field.
{HashTableStartFieldOffset, &HashTableStart, 1},
{BackPatchStartOffset, &HashTableStart, 1},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the code to use sizeof(uint64_t)

// Patch the Header.MemProfOffset (=0 for profiles without MemProf
// data).
{MemProfSectionOffset, &MemProfSectionStart, 1},
{MemProfOffset, &MemProfSectionStart, 1},
// Patch the Header.BinaryIdSectionOffset.
{BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
{BinaryIdOffset, &BinaryIdSectionStart, 1},
// Patch the Header.TemporalProfTracesOffset (=0 for profiles without
// traces).
{TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
Expand All @@ -788,12 +786,12 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
// Now do the final patch:
PatchItem PatchItems[] = {
// Patch the Header.HashOffset field.
{HashTableStartFieldOffset, &HashTableStart, 1},
{BackPatchStartOffset, &HashTableStart, 1},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, I suggest grouping these fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave them as they currently are, to make this patch focused on header writer encapsulation (which paves the way for partial forward compatibility in #88212)

// Patch the Header.MemProfOffset (=0 for profiles without MemProf
// data).
{MemProfSectionOffset, &MemProfSectionStart, 1},
{MemProfOffset, &MemProfSectionStart, 1},
// Patch the Header.BinaryIdSectionOffset.
{BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
{BinaryIdOffset, &BinaryIdSectionStart, 1},
// Patch the Header.TemporalProfTracesOffset (=0 for profiles without
// traces).
{TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
Expand Down
Loading