[llvm-profdata] Enabled functionality to write split-layout profile #101795
Conversation
|
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-pgo Author: William Junda Huang (huangjd) ChangesUsing the flag The split layout feature was already implemented in SampleProfWriter but previously there is no way to use it from llvm-profdata. Full diff: https://github.com/llvm/llvm-project/pull/101795.diff 4 Files Affected:
diff --git a/llvm/docs/CommandGuide/llvm-profdata.rst b/llvm/docs/CommandGuide/llvm-profdata.rst
index acf016a6dbcd7..166f694a2a611 100644
--- a/llvm/docs/CommandGuide/llvm-profdata.rst
+++ b/llvm/docs/CommandGuide/llvm-profdata.rst
@@ -162,6 +162,12 @@ OPTIONS
coverage for the optimized target. This option can only be used with
sample-based profile in extbinary format.
+.. option:: --split_layout=[true|false]
+
+ Split the profile data section to two with one containing sample profiles with
+ inlined functions and the other not. This option can only be used with
+ sample-based profile in extbinary format.
+
.. option:: --convert-sample-profile-layout=[nest|flat]
Convert the merged profile into a profile with a new layout. Supported
diff --git a/llvm/test/tools/llvm-profdata/Inputs/split-layout.profdata b/llvm/test/tools/llvm-profdata/Inputs/split-layout.profdata
new file mode 100644
index 0000000000000..bc473ae76558f
Binary files /dev/null and b/llvm/test/tools/llvm-profdata/Inputs/split-layout.profdata differ
diff --git a/llvm/test/tools/llvm-profdata/sample-split-layout.test b/llvm/test/tools/llvm-profdata/sample-split-layout.test
new file mode 100644
index 0000000000000..60a194e108362
--- /dev/null
+++ b/llvm/test/tools/llvm-profdata/sample-split-layout.test
@@ -0,0 +1,2 @@
+RUN: llvm-profdata merge --sample --extbinary --split_layout %p/Inputs/sample-profile.proftext -o %t-output
+RUN: diff %t-output %p/Inputs/split-layout.profdata
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 1f6c4c604d57b..1f6efa788b1b8 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -207,6 +207,12 @@ cl::opt<bool> GenPartialProfile(
"gen-partial-profile", cl::init(false), cl::Hidden,
cl::sub(MergeSubcommand),
cl::desc("Generate a partial profile (only meaningful for -extbinary)"));
+cl::opt<bool> SplitLayout(
+ "split_layout", cl::init(false), cl::Hidden,
+ cl::sub(MergeSubcommand),
+ cl::desc("Split the profile to two sections with one containing sample "
+ "profiles with inlined functions and the another not (only "
+ "meaningful for -extbinary)"));
cl::opt<std::string> SupplInstrWithSample(
"supplement-instr-with-sample", cl::init(""), cl::Hidden,
cl::sub(MergeSubcommand),
@@ -1492,6 +1498,12 @@ static void handleExtBinaryWriter(sampleprof::SampleProfileWriter &Writer,
else
Writer.setPartialProfile();
}
+ if (SplitLayout) {
+ if (OutputFormat != PF_Ext_Binary)
+ warn("-split-layout is ignored. Specify -extbinary to enable it");
+ else
+ Writer.resetSecLayout(SectionLayout::CtxSplitLayout);
+ }
}
static void mergeSampleProfile(const WeightedFileVector &Inputs,
|
huangjd
added
llvm-tools
You can test this locally with the following command:git-clang-format --diff a7ba73bf614f6d147bd1cdaddee156bd85e31703 a2c0b510e0cee06b9ef004eee4a3d12f7d9e6b99 --extensions h,cpp -- llvm/include/llvm/ProfileData/SampleProfReader.h llvm/include/llvm/ProfileData/SampleProfWriter.h llvm/lib/ProfileData/SampleProfReader.cpp llvm/lib/ProfileData/SampleProfWriter.cpp llvm/tools/llvm-profdata/llvm-profdata.cppView the diff from clang-format here.diff --git a/llvm/include/llvm/ProfileData/SampleProfWriter.h b/llvm/include/llvm/ProfileData/SampleProfWriter.h
index 4b659eaf95..cbde6cb5ab 100644
--- a/llvm/include/llvm/ProfileData/SampleProfWriter.h
+++ b/llvm/include/llvm/ProfileData/SampleProfWriter.h
@@ -176,9 +176,7 @@ protected:
return sampleprof_error::success;
}
- void setUseCtxSplitLayout() override {
- MarkFlatProfiles = true;
- }
+ void setUseCtxSplitLayout() override { MarkFlatProfiles = true; }
private:
/// Indent level to use when writing.
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 3b775de11e..a72f4dc060 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -208,8 +208,7 @@ cl::opt<bool> GenPartialProfile(
cl::sub(MergeSubcommand),
cl::desc("Generate a partial profile (only meaningful for -extbinary)"));
cl::opt<bool> SplitLayout(
- "split-layout", cl::init(false), cl::Hidden,
- cl::sub(MergeSubcommand),
+ "split-layout", cl::init(false), cl::Hidden, cl::sub(MergeSubcommand),
cl::desc("Split the profile to two sections with one containing sample "
"profiles with inlined functions and the other without (only "
"meaningful for -extbinary)"));
|
| @@ -0,0 +1,2 @@ | |||
| RUN: llvm-profdata merge --sample --extbinary --split_layout %p/Inputs/sample-profile.proftext -o %t-output | |||
| RUN: diff %t-output %p/Inputs/split-layout.profdata | |||
There was a problem hiding this comment.
Is there a way to test this without checking in the binary file split-layout.profdata?
If you must use a binary file, consider using printf to create the file so changes can be reviewed.
llvm-project/llvm/test/tools/llvm-cgdata/error.test
Lines 25 to 29 in 74125fa
There was a problem hiding this comment.
One way to avoid checking in binary file is to convert it to text format (with llvm-profdata merge --text) as input, and convert it back to binary file with a RUN line.
There was a problem hiding this comment.
Note that there is no way to represent flat profile in text mode, I will have to implement that
There was a problem hiding this comment.
And there's no point to represent that in text mode, because the point of split layout is to allow sample profile reader to skip reading an entire section when used by ThinLTO since samples without inline callsites provide no info to advise inlining. In text mode the profile is read sequentially anyways so you can't skip anything
There was a problem hiding this comment.
Note that there is no way to represent flat profile in text mode, I will have to implement that
Maybe we can try --show-sec-info-only to check the section size. The size would change if others remove what you adds.
There was a problem hiding this comment.
Checking section info only sounds good to me.
|
A drive-by comment, what's the use case to write split-layout profile? |
| @@ -0,0 +1,2 @@ | |||
| RUN: llvm-profdata merge --sample --extbinary --split_layout %p/Inputs/sample-profile.proftext -o %t-output | |||
| RUN: diff %t-output %p/Inputs/split-layout.profdata | |||
ThinLTO uses samples with inlined functions to decide whether to perform inline or not, so samples without inlined functions are useless in this stage, we gather them in one section so that they can be skipped entirely without building a funcoffsettable for them |
|
Added text mode representation for checking purpose . |
| // This metadata is only for manual inspection only. We already created a | ||
| // FunctionSamples and put it in the profile map, so there is no point | ||
| // to skip profiles even they have no use for ThinLTO. | ||
| if (Input == StringRef(" !Flat")) |
There was a problem hiding this comment.
I think we need to make sure that profiles in different formats, originating from the same source, to be equivalent. This means that if we do a round-trip conversion—from a "text" profile to an "extbinary" profile and back to a "text" profile—the two text profiles before and after the conversion should be identical. However, it seems that here the "!flat" metadata would be lost because we treat "!Flat" as a no-op, and the extended binary format won't be set to split-layout.
There was a problem hiding this comment.
Text and Extbinary mode was already not perfectly compatible. There are already various types of info that will be lost when converting Extbinary to Text, for example Profile Symbol List, flags such as PartialProfile, ProfileIsFS etc. Furthermore, the sample profile writer splits flat profiles from non-flat profiles at runtime, and the reader ignores the flag if used by llvm-profdata (instead of used by thinLTO), so the flag is a no-op in this sense, and the round trip output will be identical, unless the user intentionally marked a non-flat function as flat, which doesn't make sense anyways (profile reader could emit a warning in this case but it is also pointless, because 1. in llvm-profdata the writer will correct this, 2. in thinLTO the flat section is skipped intentionally so we don't even know there's non-flat function inside 3. in other compilation mode we read all functions with matching names regardless they are flat or not.)
There was a problem hiding this comment.
Does losing !flat change the semantics of the profile? If not, they are still equivalent? On other hand, instead of using meta data, would a comment line be sufficient?
There was a problem hiding this comment.
Makes sense, thanks for the clarification. I thought for other incompatible cases( Profile Symbol List, PartialProfile, ProfileIsFS, etc), mostly because they don't support text format, but here as the text format is added, so maybe it's not hard to support the compatibility.
|
Update change summary: split_layout -> split-layout
How was it previously used? |
There is a function in SampleProfileWriter |
Using the flag `-split_layout` in llvm-profdata merge, the output profile can write profiles with and without inlined function into two different extbinary sections (and their FuncOffsetTable too). The section without inlined functions are marked with `SecFlagFlat` and is skipped by ThinLTO because it provides no useful info. The split layout feature was already implemented in SampleProfWriter but previously there is no way from the tool to use it.
Fixed wordings refering to this flag, Previously it is described as "samples with/out context", which confuses with CSSPGO. Now they are called "samples/functions with/out inlined functions"
huangjd
force-pushed
the
llvmprofdataFlatProfile
branch
from
44c28cf to
a2c0b51
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/1865 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/3190 Here is the relevant piece of the build log for the reference |
|
I could not reproduce the error on sparcv9 because I don't have the hardware. Those test cases were fine on Linux and I could not reason how would this patch affect them. Could someone also take a look? |
|
You do, actually: the GCC Compile Farm contains a Solaris/sparcv9 system perfectly equipped to build and test LLVM. That said, I've done some investigation myself: even in exactly the same configuration as the buildbot ( I've now switched the bot to use |
Using the flag
-split_layoutin llvm-profdata merge, the output profile can write profiles with and without inlined function into two different extbinary sections (and their FuncOffsetTable too). The section without inlined functions are marked withSecFlagFlatand is skipped by ThinLTO because it provides no useful info.The split layout feature was already implemented in SampleProfWriter but previously there is no way to use it from llvm-profdata.