[PATCH] D15540: [PGO] differentiate FE instrumentation and IR level instrumentation profiles
David Li via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 15 15:35:44 PST 2015
davidxl added inline comments.
================
Comment at: include/llvm/ProfileData/InstrProfData.inc:681
@@ -680,1 +680,3 @@
+/* Profile version is always of type uint_64. We set bit 60 to 1 if if this is
+ * an IR-level instrumentaiton generated profile and 0 if this is a Clang FE
----------------
uint64_t
================
Comment at: include/llvm/ProfileData/InstrProfData.inc:682
@@ +681,3 @@
+/* Profile version is always of type uint_64. We set bit 60 to 1 if if this is
+ * an IR-level instrumentaiton generated profile and 0 if this is a Clang FE
+ * generated profile.
----------------
Why bit 60?
Also I think we should reserve perhaps 8 bits in the version so that we can use the same format for other variants of profile (for instance completely strips-off name if the user does not care about it). For that, we need to define a set of masks:
#define VARIANT_MASKS_ALL 0xf000000000000000ULL
#define VARIANT_LATE_INTR_MASK 0x1ULL
#define GET_VERSION(V) ((V)&~VARIANT_MASKS)
================
Comment at: include/llvm/ProfileData/InstrProfData.inc:687
@@ +686,3 @@
+#define SET_IR_LEVEL_PROFILE_FLAG(x) ((x | IR_LEVEL_PROFILE_FLAG))
+#define UNSET_IR_LEVEL_PROFILE_FLAG(x) ((x & ~IR_LEVEL_PROFILE_FLAG))
+#define IS_IR_LEVEL_PROFILE(x) (((x& IR_LEVEL_PROFILE_FLAG) != 0))
----------------
Why do you need to unset it?
================
Comment at: include/llvm/ProfileData/InstrProfData.inc:689
@@ -681,1 +688,3 @@
+#define IS_IR_LEVEL_PROFILE(x) (((x& IR_LEVEL_PROFILE_FLAG) != 0))
+
/* Runtime section names and name strings. */
----------------
Also remember to sync up the copy in compiler-rt of this file
================
Comment at: include/llvm/ProfileData/InstrProfReader.h:109
@@ -107,2 +108,3 @@
line_iterator Line;
+ uint64_t Version;
----------------
Text format does not have version. Make this a boolean flag.
================
Comment at: include/llvm/ProfileData/InstrProfReader.h:120
@@ -117,3 +119,3 @@
- /// Read the header.
- std::error_code readHeader() override { return success(); }
+ uint64_t getVersion() const override { return Version; };
+
----------------
Do not introduce this interface in the base class -- Version means different things across Raw and Indexed reader.
Simply add an interface to query if it is for late instr.
================
Comment at: include/llvm/ProfileData/InstrProfReader.h:141
@@ -136,2 +140,3 @@
bool ShouldSwapBytes;
+ uint64_t Version;
uint64_t CountersDelta;
----------------
Ok to add this field in implementation -- add a comment that Version value here is a OR of mask and real version.
================
Comment at: include/llvm/ProfileData/InstrProfReader.h:166
@@ -160,2 +165,3 @@
std::error_code readNextRecord(InstrProfRecord &Record) override;
+ uint64_t getVersion() const override { return Version; };
----------------
Do not expose version at interface level. Just 'isIRLevelProfile'
================
Comment at: include/llvm/ProfileData/InstrProfReader.h:331
@@ -324,3 +330,3 @@
public:
- uint64_t getVersion() const { return Index->getVersion(); }
+ uint64_t getVersion() const override { return Index->getVersion(); }
IndexedInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer)
----------------
This interface needs to be changed to mask out the flag bit -- otherwise you change the semantics of the old interface (which fortunately is not yet used).
{ return GET_VERSION(Index->getVersion()); }
Add a new interface for isIRLevelProfiling.
================
Comment at: include/llvm/ProfileData/InstrProfWriter.h:43
@@ -42,3 +42,3 @@
/// summed.
- std::error_code addRecord(InstrProfRecord &&I);
+ std::error_code addRecord(InstrProfRecord &&I, bool IsLIRInstr = false);
/// Write the profile to \c OS
----------------
make IsIRInstr a member field of the writer so that you don't need to change interfaces here.
================
Comment at: lib/ProfileData/InstrProfReader.cpp:112
@@ -111,1 +111,3 @@
+std::error_code TextInstrProfReader::readHeader() {
+ bool IsIRInstr = false;
----------------
Missing documentation.
================
Comment at: lib/ProfileData/InstrProfReader.cpp:121
@@ +120,3 @@
+ IsIRInstr = (V == 1);
+ }
+ ++Line;
----------------
Report error if V is not 0 nor 1.
================
Comment at: lib/ProfileData/InstrProfReader.cpp:224
@@ -206,1 +223,3 @@
+ Version = swap(Header.Version);
+ if (UNSET_IR_LEVEL_PROFILE_FLAG(Version) != RawInstrProf::Version)
return error(instrprof_error::unsupported_version);
----------------
Do not use 'UNSET'..., but use GET_VERSION macro I suggested.
================
Comment at: lib/ProfileData/InstrProfReader.cpp:523
@@ -503,3 +522,3 @@
uint64_t FormatVersion = endian::byte_swap<uint64_t, little>(Header->Version);
- if (FormatVersion > IndexedInstrProf::Version)
+ if (UNSET_IR_LEVEL_PROFILE_FLAG(FormatVersion) > IndexedInstrProf::Version)
return error(instrprof_error::unsupported_version);
----------------
GET_VERSION(...)
================
Comment at: lib/ProfileData/InstrProfWriter.cpp:127
@@ +126,3 @@
+ } else {
+ for (auto &IT : I.Counts)
+ if (IT > MaxFunctionCount)
----------------
Check Dest's Counts with merged values.
================
Comment at: lib/ProfileData/InstrProfWriter.cpp:150
@@ +149,3 @@
+ if (IsIRInstr)
+ Header.Version = SET_IR_LEVEL_PROFILE_FLAG(IndexedInstrProf::Version);
+ else
----------------
It is actually more readable with direct IndexedInstrProf::version | IR_PROF_VARIANT_MASK
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:669
@@ -668,1 +668,3 @@
bool PGOInstrumentationGen::runOnModule(Module &M) {
+ // Create a COMDAT variable __llvm_profile_ir_level to let runtime
+ // aware this is ir_level profile so it can set the version flag.
----------------
may be a helper function for this.
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:675
@@ +674,3 @@
+ Constant::getIntegerValue(IntTy32, APInt(32, 1)),
+ "__llvm_profile_ir_level");
+ IRLevelFlagVariable->setVisibility(GlobalValue::DefaultVisibility);
----------------
As it is shared, put the string macro in InstrProfData.inc -- see other section strings.
Also add an interface in InstrProf.h to retrieve the string -- do not hard code string here.
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:677
@@ +676,3 @@
+ IRLevelFlagVariable->setVisibility(GlobalValue::DefaultVisibility);
+ IRLevelFlagVariable->setAlignment(8);
+ IRLevelFlagVariable->setComdat(
----------------
Why align 8?
================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:679
@@ +678,3 @@
+ IRLevelFlagVariable->setComdat(
+ M.getOrInsertComdat(StringRef("__llvm_profile_ir_level")));
+
----------------
No literal string
================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:120
@@ -117,1 +119,3 @@
auto Reader = std::move(ReaderOrErr.get());
+ uint64_t Version = Reader->getVersion();
+ bool IsIRProfile = IS_IR_LEVEL_PROFILE(Version);
----------------
Simplify the code like this:
1) introduce a interface in writer:
setIsIRLevelProfile (bool true|false); On writer construction, the value is 'unknown'. When the set interface is called, it will check if it is set and compare previous value with current value -- reports Error when incosistence is detected.
2) the code will be just:
if (std::error_code EC = Writer.setIsIRLevelProfile(Reader->isIRLevelProfile())
exitWithError("....);
http://reviews.llvm.org/D15540
More information about the llvm-commits
mailing list