-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Introduction of typified section in ExtBinary format #166553
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-pgo Author: Sergey Shcherbinin (SergeyShch01) ChangesNew sections are introduced for storing profile of vairous types:
The only change in the structure of ExtBinary is a substitution of SecLBRProfile and SecFuncOffsetTable sections by new (typified) ones, so, to void duplication of layout description and handling, all changes are done on low-level: section types are modified in the existing layout right before profile writing. No new profile types are introduced - this change is rather a preparation for future profiling extentions: only LBR profile can be inside currently. Processing of VTable profile is unmodified. Extended description is here: https://discourse.llvm.org/t/rfc-typifiedprofilesection-generalized-per-function-profile-blocks-in-extbinary-format/87881 Testing of the new scenario is added to llvm/test/tools/llvm-profdata/sample-flatten-profile.test (--extbinary-force-typified-prof forces usage of typified sections). Full diff: https://github.com/llvm/llvm-project/pull/166553.diff 6 Files Affected:
diff --git a/llvm/include/llvm/ProfileData/SampleProf.h b/llvm/include/llvm/ProfileData/SampleProf.h
index 3dd34aba2d716..a9417dae93f81 100644
--- a/llvm/include/llvm/ProfileData/SampleProf.h
+++ b/llvm/include/llvm/ProfileData/SampleProf.h
@@ -128,9 +128,13 @@ enum SecType {
SecFuncOffsetTable = 4,
SecFuncMetadata = 5,
SecCSNameTable = 6,
+ // Substitution to SecFuncOffsetTable when we have non-LBR profile types
+ SecTypifiedFuncOffsetTable = 7,
// marker for the first type of profile.
SecFuncProfileFirst = 32,
- SecLBRProfile = SecFuncProfileFirst
+ SecLBRProfile = SecFuncProfileFirst,
+ // Substitution to SecLBRProfile when we have non-LBR profile types
+ SecTypifiedProfile = 33
};
static inline std::string getSecName(SecType Type) {
@@ -149,13 +153,20 @@ static inline std::string getSecName(SecType Type) {
return "FunctionMetadata";
case SecCSNameTable:
return "CSNameTableSection";
+ case SecTypifiedFuncOffsetTable:
+ return "TypifiedFuncOffsetTableSection";
case SecLBRProfile:
return "LBRProfileSection";
+ case SecTypifiedProfile:
+ return "TypifiedProfileSection";
default:
return "UnknownSection";
}
}
+// Types of sample profile which can be in placed in SecTypifiedProfile
+enum ProfTypes { ProfTypeLBR = 0, ProfTypeNum };
+
// Entry type of section header table used by SampleProfileExtBinaryBaseReader
// and SampleProfileExtBinaryBaseWriter.
struct SecHdrTableEntry {
@@ -1341,6 +1352,12 @@ class FunctionSamples {
return !(*this == Other);
}
+ bool hasNonLBRSamples() const {
+ // currently just a stub - should be implemented when
+ // first non-LBR profile is encountered
+ return false;
+ }
+
private:
/// CFG hash value for the function.
uint64_t FunctionHash = 0;
@@ -1466,6 +1483,15 @@ class SampleProfileMap
size_t erase(const key_type &Key) { return base_type::erase(Key); }
iterator erase(iterator It) { return base_type::erase(It); }
+
+ bool hasNonLBRProfile() const {
+ for (const auto &[Context, FuncSamples] : *this) {
+ if (FuncSamples.hasNonLBRSamples()) {
+ return true;
+ }
+ }
+ return false;
+ }
};
using NameFunctionSamples = std::pair<hash_code, const FunctionSamples *>;
diff --git a/llvm/include/llvm/ProfileData/SampleProfReader.h b/llvm/include/llvm/ProfileData/SampleProfReader.h
index 799938ab901c1..74e3f1773656d 100644
--- a/llvm/include/llvm/ProfileData/SampleProfReader.h
+++ b/llvm/include/llvm/ProfileData/SampleProfReader.h
@@ -570,6 +570,7 @@ class SampleProfileReader {
FuncMetadataIndex;
std::pair<const uint8_t *, const uint8_t *> ProfileSecRange;
+ bool IsProfileTypified = false;
/// Whether the profile has attribute metadata.
bool ProfileHasAttribute = false;
@@ -687,6 +688,10 @@ class LLVM_ABI SampleProfileReaderBinary : public SampleProfileReader {
/// Read the contents of the given profile instance.
std::error_code readProfile(FunctionSamples &FProfile);
+ /// Read specific profile types.
+ std::error_code readLBRProfile(FunctionSamples &FProfile);
+ std::error_code readTypifiedProfile(FunctionSamples &FProfile);
+
/// Read the contents of Magic number and Version number.
std::error_code readMagicIdent();
diff --git a/llvm/include/llvm/ProfileData/SampleProfWriter.h b/llvm/include/llvm/ProfileData/SampleProfWriter.h
index 9dbeaf56509b0..052843f9f55ac 100644
--- a/llvm/include/llvm/ProfileData/SampleProfWriter.h
+++ b/llvm/include/llvm/ProfileData/SampleProfWriter.h
@@ -215,6 +215,10 @@ class LLVM_ABI SampleProfileWriterBinary : public SampleProfileWriter {
virtual std::error_code writeContextIdx(const SampleContext &Context);
std::error_code writeNameIdx(FunctionId FName);
std::error_code writeBody(const FunctionSamples &S);
+ void writeLBRProfile(const FunctionSamples &S);
+ /// Writes typified profile for function
+ void writeTypifiedProfile(const FunctionSamples &S);
+
inline void stablizeNameTable(MapVector<FunctionId, uint32_t> &NameTable,
std::set<FunctionId> &V);
@@ -230,6 +234,7 @@ class LLVM_ABI SampleProfileWriterBinary : public SampleProfileWriter {
raw_ostream &OS);
bool WriteVTableProf = false;
+ bool WriteTypifiedProf = false;
private:
LLVM_ABI friend ErrorOr<std::unique_ptr<SampleProfileWriter>>
@@ -427,6 +432,9 @@ class LLVM_ABI SampleProfileWriterExtBinary
std::error_code writeSections(const SampleProfileMap &ProfileMap) override;
+ // Configure whether to use typified profile sections
+ void configureTypifiedProfile(const SampleProfileMap &ProfileMap);
+
std::error_code writeCustomSection(SecType Type) override {
return sampleprof_error::success;
};
@@ -435,6 +443,11 @@ class LLVM_ABI SampleProfileWriterExtBinary
assert((SL == DefaultLayout || SL == CtxSplitLayout) &&
"Unsupported layout");
}
+
+ /// Section types for profile storage and bookkeeping (used to switch between
+ /// typified and non-typified profiles).
+ SecType ProfSection = SecLBRProfile;
+ SecType FuncOffsetSection = SecFuncOffsetTable;
};
} // end namespace sampleprof
diff --git a/llvm/lib/ProfileData/SampleProfReader.cpp b/llvm/lib/ProfileData/SampleProfReader.cpp
index 766c0814ca067..d97cc8a6f6b88 100644
--- a/llvm/lib/ProfileData/SampleProfReader.cpp
+++ b/llvm/lib/ProfileData/SampleProfReader.cpp
@@ -710,7 +710,7 @@ SampleProfileReaderBinary::readCallsiteVTableProf(FunctionSamples &FProfile) {
}
std::error_code
-SampleProfileReaderBinary::readProfile(FunctionSamples &FProfile) {
+SampleProfileReaderBinary::readLBRProfile(FunctionSamples &FProfile) {
auto NumSamples = readNumber<uint64_t>();
if (std::error_code EC = NumSamples.getError())
return EC;
@@ -761,6 +761,52 @@ SampleProfileReaderBinary::readProfile(FunctionSamples &FProfile) {
FProfile.addBodySamples(*LineOffset, DiscriminatorVal, *NumSamples);
}
+ return sampleprof_error::success;
+}
+
+std::error_code
+SampleProfileReaderBinary::readTypifiedProfile(FunctionSamples &FProfile) {
+ // read the number of profile types
+ auto ProfNum = readNumber<uint64_t>();
+ if (std::error_code EC = ProfNum.getError())
+ return EC;
+
+ // read specified number of typified profiles
+ for (uint64_t i = 0; i < *ProfNum; i++) {
+ auto Type = readNumber<uint64_t>();
+ if (std::error_code EC = Type.getError())
+ return EC;
+ auto Size = readUnencodedNumber<uint64_t>();
+ if (std::error_code EC = Size.getError())
+ return EC;
+
+ switch (*Type) {
+ case ProfTypeLBR:
+ if (std::error_code EC = readLBRProfile(FProfile))
+ return EC;
+ break;
+ default:
+ // skip unknown profile type for forward compatibility
+ Data += *Size;
+ if (Data > End)
+ return sampleprof_error::truncated;
+ break;
+ }
+ }
+
+ return sampleprof_error::success;
+}
+
+std::error_code
+SampleProfileReaderBinary::readProfile(FunctionSamples &FProfile) {
+ if (IsProfileTypified) {
+ if (std::error_code EC = readTypifiedProfile(FProfile))
+ return EC;
+ } else {
+ if (std::error_code EC = readLBRProfile(FProfile))
+ return EC;
+ }
+
// Read all the samples for inlined function calls.
auto NumCallsites = readNumber<uint32_t>();
if (std::error_code EC = NumCallsites.getError())
@@ -876,11 +922,14 @@ std::error_code SampleProfileReaderExtBinaryBase::readOneSection(
break;
}
case SecLBRProfile:
+ case SecTypifiedProfile:
ProfileSecRange = std::make_pair(Data, End);
+ IsProfileTypified = Entry.Type == SecTypifiedProfile;
if (std::error_code EC = readFuncProfiles())
return EC;
break;
case SecFuncOffsetTable:
+ case SecTypifiedFuncOffsetTable:
// If module is absent, we are using LLVM tools, and need to read all
// profiles, so skip reading the function offset table.
if (!M) {
diff --git a/llvm/lib/ProfileData/SampleProfWriter.cpp b/llvm/lib/ProfileData/SampleProfWriter.cpp
index e5f31348578b8..c9787f367156c 100644
--- a/llvm/lib/ProfileData/SampleProfWriter.cpp
+++ b/llvm/lib/ProfileData/SampleProfWriter.cpp
@@ -46,6 +46,10 @@ static cl::opt<bool> ExtBinaryWriteVTableTypeProf(
"extbinary-write-vtable-type-prof", cl::init(false), cl::Hidden,
cl::desc("Write vtable type profile in ext-binary sample profile writer"));
+static cl::opt<bool> ExtBinaryForceTypifiedProf(
+ "extbinary-force-typified-prof", cl::init(false), cl::Hidden,
+ cl::desc("Force utilization of typified profile format"));
+
namespace llvm {
namespace support {
namespace endian {
@@ -460,11 +464,13 @@ std::error_code SampleProfileWriterExtBinaryBase::writeOneSection(
return EC;
break;
case SecLBRProfile:
+ case SecTypifiedProfile:
SecLBRProfileStart = OutputStream->tell();
if (std::error_code EC = writeFuncProfiles(ProfileMap))
return EC;
break;
case SecFuncOffsetTable:
+ case SecTypifiedFuncOffsetTable:
if (auto EC = writeFuncOffsetTable())
return EC;
break;
@@ -504,11 +510,11 @@ std::error_code SampleProfileWriterExtBinary::writeDefaultLayout(
return EC;
if (auto EC = writeOneSection(SecCSNameTable, 2, ProfileMap))
return EC;
- if (auto EC = writeOneSection(SecLBRProfile, 4, ProfileMap))
+ if (auto EC = writeOneSection(ProfSection, 4, ProfileMap))
return EC;
if (auto EC = writeOneSection(SecProfileSymbolList, 5, ProfileMap))
return EC;
- if (auto EC = writeOneSection(SecFuncOffsetTable, 3, ProfileMap))
+ if (auto EC = writeOneSection(FuncOffsetSection, 3, ProfileMap))
return EC;
if (auto EC = writeOneSection(SecFuncMetadata, 6, ProfileMap))
return EC;
@@ -530,24 +536,23 @@ std::error_code SampleProfileWriterExtBinary::writeCtxSplitLayout(
const SampleProfileMap &ProfileMap) {
SampleProfileMap ContextProfileMap, NoContextProfileMap;
splitProfileMapToTwo(ProfileMap, ContextProfileMap, NoContextProfileMap);
-
if (auto EC = writeOneSection(SecProfSummary, 0, ProfileMap))
return EC;
if (auto EC = writeOneSection(SecNameTable, 1, ProfileMap))
return EC;
- if (auto EC = writeOneSection(SecLBRProfile, 3, ContextProfileMap))
+ if (auto EC = writeOneSection(ProfSection, 3, ContextProfileMap))
return EC;
- if (auto EC = writeOneSection(SecFuncOffsetTable, 2, ContextProfileMap))
+ if (auto EC = writeOneSection(FuncOffsetSection, 2, ContextProfileMap))
return EC;
// Mark the section to have no context. Note section flag needs to be set
// before writing the section.
addSectionFlag(5, SecCommonFlags::SecFlagFlat);
- if (auto EC = writeOneSection(SecLBRProfile, 5, NoContextProfileMap))
+ if (auto EC = writeOneSection(ProfSection, 5, NoContextProfileMap))
return EC;
// Mark the section to have no context. Note section flag needs to be set
// before writing the section.
addSectionFlag(4, SecCommonFlags::SecFlagFlat);
- if (auto EC = writeOneSection(SecFuncOffsetTable, 4, NoContextProfileMap))
+ if (auto EC = writeOneSection(FuncOffsetSection, 4, NoContextProfileMap))
return EC;
if (auto EC = writeOneSection(SecProfileSymbolList, 6, ProfileMap))
return EC;
@@ -557,8 +562,30 @@ std::error_code SampleProfileWriterExtBinary::writeCtxSplitLayout(
return sampleprof_error::success;
}
+void SampleProfileWriterExtBinary::configureTypifiedProfile(
+ const SampleProfileMap &ProfileMap) {
+ if (!ExtBinaryForceTypifiedProf && !ProfileMap.hasNonLBRProfile()) {
+ WriteTypifiedProf = false;
+ ProfSection = SecLBRProfile;
+ FuncOffsetSection = SecFuncOffsetTable;
+ return;
+ }
+ // Use typified profile sections: directly change the section types
+ // to avoid duplicating the whole layout and its handling.
+ WriteTypifiedProf = true;
+ FuncOffsetSection = SecTypifiedFuncOffsetTable;
+ ProfSection = SecTypifiedProfile;
+ for (auto &Entry : SectionHdrLayout) {
+ if (Entry.Type == SecFuncOffsetTable)
+ Entry.Type = SecTypifiedFuncOffsetTable;
+ else if (Entry.Type == SecLBRProfile)
+ Entry.Type = SecTypifiedProfile;
+ }
+}
+
std::error_code SampleProfileWriterExtBinary::writeSections(
const SampleProfileMap &ProfileMap) {
+ configureTypifiedProfile(ProfileMap);
std::error_code EC;
if (SecLayout == DefaultLayout)
EC = writeDefaultLayout(ProfileMap);
@@ -885,14 +912,10 @@ std::error_code SampleProfileWriterBinary::writeSummary() {
}
return sampleprof_error::success;
}
-std::error_code SampleProfileWriterBinary::writeBody(const FunctionSamples &S) {
- auto &OS = *OutputStream;
- if (std::error_code EC = writeContextIdx(S.getContext()))
- return EC;
+void SampleProfileWriterBinary::writeLBRProfile(const FunctionSamples &S) {
+ auto &OS = *OutputStream;
encodeULEB128(S.getTotalSamples(), OS);
-
- // Emit all the body samples.
encodeULEB128(S.getBodySamples().size(), OS);
for (const auto &I : S.getBodySamples()) {
LineLocation Loc = I.first;
@@ -900,6 +923,39 @@ std::error_code SampleProfileWriterBinary::writeBody(const FunctionSamples &S) {
Loc.serialize(OS);
Sample.serialize(OS, getNameTable());
}
+}
+
+void SampleProfileWriterBinary::writeTypifiedProfile(const FunctionSamples &S) {
+ // Currently only LBR profile is supported as typified profile.
+ auto &OS = *OutputStream;
+ // write the number of profile types for function
+ encodeULEB128(1, OS);
+ // Start first profile writing: write profile type
+ encodeULEB128(ProfTypeLBR, OS);
+ // create placeholder for profile size
+ uint64_t SizeOffset = OS.tell();
+ support::endian::Writer PlaceWriter(OS, llvm::endianness::little);
+ PlaceWriter.write(static_cast<uint64_t>(-1));
+ // write the profile itself
+ uint64_t BodyStart = OS.tell();
+ writeLBRProfile(S);
+ uint64_t BodySize = OS.tell() - BodyStart;
+ // write profile size
+ support::endian::SeekableWriter PWriter(static_cast<raw_pwrite_stream &>(OS),
+ llvm::endianness::little);
+ PWriter.pwrite(BodySize, SizeOffset);
+}
+
+std::error_code SampleProfileWriterBinary::writeBody(const FunctionSamples &S) {
+ auto &OS = *OutputStream;
+ if (std::error_code EC = writeContextIdx(S.getContext()))
+ return EC;
+
+ // Emit all the body samples.
+ if (WriteTypifiedProf)
+ writeTypifiedProfile(S);
+ else
+ writeLBRProfile(S);
// Recursively emit all the callsite samples.
uint64_t NumCallsites = 0;
diff --git a/llvm/test/tools/llvm-profdata/sample-flatten-profile.test b/llvm/test/tools/llvm-profdata/sample-flatten-profile.test
index f99021bc6b723..34dab716a067f 100644
--- a/llvm/test/tools/llvm-profdata/sample-flatten-profile.test
+++ b/llvm/test/tools/llvm-profdata/sample-flatten-profile.test
@@ -1,8 +1,10 @@
; RUN: llvm-profdata merge --sample --convert-sample-profile-layout=flat --text %S/Inputs/sample-flatten-profile.proftext -o - | FileCheck %s --match-full-lines --strict-whitespace
; RUN: llvm-profdata merge --sample --extbinary %S/Inputs/sample-flatten-profile.proftext -o %t2 && llvm-profdata merge --sample --convert-sample-profile-layout=flat --text %t2 -o - | FileCheck %s --match-full-lines --strict-whitespace
+; RUN: llvm-profdata merge --sample --extbinary-force-typified-prof --extbinary %S/Inputs/sample-flatten-profile.proftext -o %t2 && llvm-profdata merge --sample --convert-sample-profile-layout=flat --text %t2 -o - | FileCheck %s --match-full-lines --strict-whitespace
; RUN: llvm-profdata merge --sample --convert-sample-profile-layout=flat --text %S/Inputs/sample-flatten-profile-cs.proftext -o - | FileCheck %s --match-full-lines --strict-whitespace --check-prefix=CHECK-CS
; RUN: llvm-profdata merge --sample --extbinary %S/Inputs/sample-flatten-profile-cs.proftext -o %t2 && llvm-profdata merge --sample --convert-sample-profile-layout=flat --text %t2 -o - | FileCheck %s --match-full-lines --strict-whitespace --check-prefix=CHECK-CS
+; RUN: llvm-profdata merge --sample --extbinary-force-typified-prof --extbinary %S/Inputs/sample-flatten-profile-cs.proftext -o %t2 && llvm-profdata merge --sample --convert-sample-profile-layout=flat --text %t2 -o - | FileCheck %s --match-full-lines --strict-whitespace --check-prefix=CHECK-CS
; CHECK:baz:169:10
; CHECK-NEXT: 1: 10
|
mingmingl-llvm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work and RFC. I requested review from some other folks working on SamplePGO for visibility and comments.
As a thought experiment, if a new type associate new profile type to IR with current LBR anchors (line:discriminator or pseudo probe), how will the storage representation look like?
|
@mtrofin can you please also have a look? |
Do you think there can be issues w/ the strightforward approach? I was thinking about adding a new map to FunctionSamples (for [anchor -> prof_value] correspondence). |
I'm in favor of enriching profiles for new optimizations and onboard with the idea of pursuing forward compatibility to make new changes easier. Could you illustrate on how a new type of profile is mapped onto the IR and how it will be represented in the both memory (class structures) and serialized format? |
|
Well, let's take load latency profile as an example: a profiled latency value is given to a load. Load can be pointed to by line:descriminator (probe doesn't work). Also, such feedback can be unrelated to call context (which can be important in LBR case). In ExtBinary file a new profile type is introduced (say ProfTypeLoadLat) and then it will be possible to have a sub-record inside a function record in SecTypifiedProfile section for this new profile type. This sub-record for ProfTypeLoadLat can contain a list of [line:descriminator, profiled_latency] pairs. Then, when ExtBinary file is loaded, this new profile is read and [line:descriminator, profiled_latency] pairs are moved to a new map in FunctionSamples (e.g. std::map<LineLocation, ProfiledLoadLat> LoadLatencies). Then latencies from LoadLatencies can be put to instruction metadata in LLVM IR. Generally this should look similar to current LBR feedback processing. |
How will be |
|
|
||
| std::error_code | ||
| SampleProfileReaderBinary::readTypifiedProfile(FunctionSamples &FProfile) { | ||
| // read the number of profile types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Per https://llvm.org/docs/CodingStandards.html#commenting, When writing comments, write them as English prose, using proper capitalization, punctuation, etc Could you please capitalize the start of the comments and add a period to end a sentence if applicable in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done
|
|
||
| void SampleProfileWriterExtBinary::configureTypifiedProfile( | ||
| const SampleProfileMap &ProfileMap) { | ||
| if (!ExtBinaryForceTypifiedProf && !ProfileMap.hasNonLBRProfile()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR supports both the current format and the new typified format, and the default is to use the current format unless 1) a non-LBR profile is present, and 2) the extbinary-force-typified-prof flag is used.
Is the goal to replace the current format with the typified format? It seems having both adds some code complexity (e.g., for new use cases), and I'm wondering if we should aim at converging to one upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the goal to replace the current format with the typified format?
I believe this is a goal (if nobody objects). At some point we could invert the extbinary-force-typified-prof knob and have the typified format set as a default.
However we should keep backward compatibility - then the current format should be also supported.
| // Currently only LBR profile is supported as typified profile. | ||
| auto &OS = *OutputStream; | ||
| // write the number of profile types for function | ||
| encodeULEB128(1, OS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: what do you think of avoiding a magic number as the number of typified profile? For instance, one way is to repurpose FunctionSamples::hasNonLBRSamples to return the number of typified profiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I got the idea.
This is a number of profiles which are really encoded for the function (this number will be used during the profile reading).
I updated this code to make this clearer.
| if (std::error_code EC = writeContextIdx(S.getContext())) | ||
| return EC; | ||
|
|
||
| // Emit all the body samples. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that only the body samples are being wrapped in the new typified format. Other serialized fields within FunctionSamples, such as callsite samples (between L960 and L970, and also derived from LBR), are still written outside of it. This means the overall format isn't fully forward-compatible.
To make this more generalized for the future, should we consider serializing the entire FunctionSamples object within the new typified block, rather than just the body samples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my mind, the callsite samples corresponds to profile collected in (inlined) callees then it also can include not just LBR samples but other profile types as well.
Here we recursively invoke the same writeBody() which can also write typified profile. If some profile type is absent for callees then it won't be written at this point.
This shouldn't break the forward compatibility as this is just a way to specify profile for nested calls which isn't directly related to LBR.
If you have different understanding then please share your ideas.
However, there are other LBR-related data which is outside of typified LBR section: this is S.getHeadSamples() (have a look at SampleProfileWriterExtBinaryBase::writeSample() for details). I left it here to avoid too bit code disturbtion. But if it looks acceptable I can also move this data to typified LBR section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved HeadSamples to the typified LBR sub-section by a separate commit. This added more complexity to the code but currently the typified format is fully consistent.
| /// | ||
| /// The format used here is more structured and deliberate because | ||
| /// it needs to be parsed by the SampleProfileReaderText class. | ||
| std::error_code SampleProfileWriterText::writeSample(const FunctionSamples &S) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a human-readable version of the typified profile would be very helpful for debugging.
Do you have any plans to add this in a follow-up change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe each new profile type should be supported in llvm-profdata (which can print the profile in text form).
However it looks to be a functionality related to new profile types (not to the typified profile format itself).
Please let me know if I've misuderstood your concern.
Co-authored-by: Mingming Liu <minglotus6@gmail.com>
+ Comments LLVM-fication + Typified profile writing is split into several interfaces to support future introduction of new profile types
The default serialization method is as follows: Assuming that the encoded size is expected to be small (as LineOffset and Discriminator are usually small) and that the shared line:discriminator pairs are expected to be reused unfrequently by different profile types (as different profile types are expected to relate to instructions of different types which usually correspond to different line:discriminator pairs) I would say that introduction of the shared In future the situation could be different and such storage could be supported (this shouldn't be difficult if we can easily introduce new profile types). |
Not sure how I can help, I'm not quite involved with sample profiling. I think the reviewer list has the appropriate folks. |
New sections are introduced for storing profile of vairous types:
The only change in the structure of ExtBinary is a substitution of SecLBRProfile and SecFuncOffsetTable sections by new (typified) ones, so, to void duplication of layout description and handling, all changes are done on low-level: section types are modified in the existing layout right before profile writing.
No new profile types are introduced - this change is rather a preparation for future profiling extentions: only LBR profile can be inside currently.
Processing of VTable profile is unmodified.
Extended description is here: https://discourse.llvm.org/t/rfc-typifiedprofilesection-generalized-per-function-profile-blocks-in-extbinary-format/87881
Testing of the new scenario is added to llvm/test/tools/llvm-profdata/sample-flatten-profile.test (--extbinary-force-typified-prof forces usage of typified sections).