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 3 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
21 changes: 21 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 @@ -197,13 +197,34 @@ class InstrProfWriter {
const OverlapFuncFilters &FuncFilter);

private:
// A profile (with header and payloads) is written out in one pass; profile
// header records the byte offsets of individual payload sections.
// When the byte size of a payload section is not known before being written
// into the output stream, profile writer reserves space for the byte offset
// of this section to allow back patching at the specified offset later.
//
// This struct is produced by `InstrProfWriter::writeHeader` to store the byte
// offset of header fields, and used later for back patching.
struct HeaderFieldOffsets {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

uint64_t HashTableStartFieldOffset;
uint64_t MemProfSectionOffset;
uint64_t BinaryIdSectionOffset;
uint64_t TemporalProfTracesOffset;
uint64_t VTableNamesOffset;
};
void addRecord(StringRef Name, uint64_t Hash, InstrProfRecord &&I,
uint64_t Weight, function_ref<void(Error)> Warn);
bool shouldEncodeData(const ProfilingData &PD);
/// Add \p Trace using reservoir sampling.
void addTemporalProfileTrace(TemporalProfTraceTy Trace);

Error writeImpl(ProfOStream &OS);

// Writes known header fields and preserves space for fields whose value are
// known only after payloads are written.
// Returns the number of bytes written and outputs the byte offset for back
// patching.
uint64_t writeHeader(ProfOStream &OS, HeaderFieldOffsets &Offsets);
};

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

Error InstrProfWriter::writeImpl(ProfOStream &OS) {
using namespace IndexedInstrProf;
using namespace support;

OnDiskChainedHashTableGenerator<InstrProfRecordWriterTrait> Generator;

InstrProfSummaryBuilder ISB(ProfileSummaryBuilder::DefaultCutoffs);
InfoObj->SummaryBuilder = &ISB;
InstrProfSummaryBuilder CSISB(ProfileSummaryBuilder::DefaultCutoffs);
InfoObj->CSSummaryBuilder = &CSISB;

// Populate the hash table generator.
SmallVector<std::pair<StringRef, const ProfilingData *>, 0> OrderedData;
for (const auto &I : FunctionData)
if (shouldEncodeData(I.getValue()))
OrderedData.emplace_back((I.getKey()), &I.getValue());
llvm::sort(OrderedData, less_first());
for (const auto &I : OrderedData)
Generator.insert(I.first, I.second);

uint64_t InstrProfWriter::writeHeader(ProfOStream &OS,
HeaderFieldOffsets &Offsets) {
// Records the offset before writing any fields.
const uint64_t StartOffset = OS.tell();
// Write the header.
IndexedInstrProf::Header Header;
Header.Magic = IndexedInstrProf::Magic;
Expand Down Expand Up @@ -612,30 +596,58 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
OS.write(reinterpret_cast<uint64_t *>(&Header)[I]);

// Save the location of Header.HashOffset field in \c OS.
uint64_t HashTableStartFieldOffset = OS.tell();
Offsets.HashTableStartFieldOffset = OS.tell();
// Reserve the space for HashOffset field.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could we shorten comments?

// Reserve space for offsets to be patched later.
OS.write(0);  // MemProf
OS.write(0);  // BinaryIdOffset
OS.write(0);  // Temporal profile
 if (!WritePrevVersion)
  OS.write(0);  // VTable names

This way, the code looks like a table and avoids repetitive comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

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();
Offsets.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();
Offsets.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();
Offsets.TemporalProfTracesOffset = OS.tell();
OS.write(0);

uint64_t VTableNamesOffset = OS.tell();
Offsets.VTableNamesOffset = OS.tell();
if (!WritePrevVersion)
OS.write(0);

return OS.tell() - StartOffset;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

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 return BackPatchStartOffset; instead?

The caller can easily compute OS.tell() - StartOffset on its own. Also, the expression is simple enough and self explanatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

}

Error InstrProfWriter::writeImpl(ProfOStream &OS) {
HeaderFieldOffsets Offsets;

// FIXME: The return value will be used in a future patch.
(void)this->writeHeader(OS, Offsets);

using namespace IndexedInstrProf;
using namespace support;

OnDiskChainedHashTableGenerator<InstrProfRecordWriterTrait> Generator;

InstrProfSummaryBuilder ISB(ProfileSummaryBuilder::DefaultCutoffs);
InfoObj->SummaryBuilder = &ISB;
InstrProfSummaryBuilder CSISB(ProfileSummaryBuilder::DefaultCutoffs);
InfoObj->CSSummaryBuilder = &CSISB;

// Populate the hash table generator.
SmallVector<std::pair<StringRef, const ProfilingData *>> OrderedData;
for (const auto &I : FunctionData)
if (shouldEncodeData(I.getValue()))
OrderedData.emplace_back((I.getKey()), &I.getValue());
llvm::sort(OrderedData, less_first());
for (const auto &I : OrderedData)
Generator.insert(I.first, I.second);

// Reserve space to write profile summary data.
uint32_t NumEntries = ProfileSummaryBuilder::DefaultCutoffs.size();
uint32_t SummarySize = Summary::getSize(Summary::NumKinds, NumEntries);
Expand Down Expand Up @@ -767,16 +779,16 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
// Now do the final patch:
PatchItem PatchItems[] = {
// Patch the Header.HashOffset field.
{HashTableStartFieldOffset, &HashTableStart, 1},
{Offsets.HashTableStartFieldOffset, &HashTableStart, 1},
// Patch the Header.MemProfOffset (=0 for profiles without MemProf
// data).
{MemProfSectionOffset, &MemProfSectionStart, 1},
{Offsets.MemProfSectionOffset, &MemProfSectionStart, 1},
// Patch the Header.BinaryIdSectionOffset.
{BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
{Offsets.BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
// Patch the Header.TemporalProfTracesOffset (=0 for profiles without
// traces).
{TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
{VTableNamesOffset, &VTableNamesSectionStart, 1},
{Offsets.TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
{Offsets.VTableNamesOffset, &VTableNamesSectionStart, 1},
// Patch the summary data.
{SummaryOffset, reinterpret_cast<uint64_t *>(TheSummary.get()),
(int)(SummarySize / sizeof(uint64_t))},
Expand All @@ -788,15 +800,15 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
// Now do the final patch:
PatchItem PatchItems[] = {
// Patch the Header.HashOffset field.
{HashTableStartFieldOffset, &HashTableStart, 1},
{Offsets.HashTableStartFieldOffset, &HashTableStart, 1},
// Patch the Header.MemProfOffset (=0 for profiles without MemProf
// data).
{MemProfSectionOffset, &MemProfSectionStart, 1},
{Offsets.MemProfSectionOffset, &MemProfSectionStart, 1},
// Patch the Header.BinaryIdSectionOffset.
{BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
{Offsets.BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
// Patch the Header.TemporalProfTracesOffset (=0 for profiles without
// traces).
{TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
{Offsets.TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
// Patch the summary data.
{SummaryOffset, reinterpret_cast<uint64_t *>(TheSummary.get()),
(int)(SummarySize / sizeof(uint64_t))},
Expand Down
Loading