[PATCH] D15540: [PGO] differentiate FE instrumentation and IR level instrumentation profiles

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/ProfileData/InstrProfData.inc:681
@@ -680,1 +680,3 @@
 
+/* Profile version is always of type uint_64_t. Reserve the upper 8 bits in the
+ * version for other variants of profile. We set the lowest bit of the upper 8
----------------
typo : uint64_t, not uint_64_t.

================
Comment at: include/llvm/ProfileData/InstrProfData.inc:691
@@ +690,3 @@
+#define IR_LEVEL_PROF_STRINGFY(s) #s
+#define IR_LEVEL_PROF_STRING(s) IR_LEVEL_PROF_STRINGFY(s)
+
----------------
Use existing macro: INSTR_PROF_QUOTE, no need for a new helper macro.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:146
@@ -141,1 +145,3 @@
   bool ShouldSwapBytes;
+  // Version of the Raw profile. It contains the variant profile information.
+  uint64_t Version;
----------------
Raw --> raw

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:322
@@ -312,3 +321,3 @@
   ~InstrProfReaderIndex() override {}
-  uint64_t getVersion() const override { return FormatVersion; }
+  uint64_t getVersion() const { return FormatVersion; }
 };
----------------
do not remove override here -- note that getVersion here is for indexed format class hierarchy (to support different indexed formats ). 

Note that Version really means format version, so the masking needs to be applied here too.

Add another interface to check isIRLevelProfiling.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:322
@@ -312,3 +321,3 @@
   ~InstrProfReaderIndex() override {}
-  uint64_t getVersion() const override { return FormatVersion; }
+  uint64_t getVersion() const { return FormatVersion; }
 };
----------------
davidxl wrote:
> do not remove override here -- note that getVersion here is for indexed format class hierarchy (to support different indexed formats ). 
> 
> Note that Version really means format version, so the masking needs to be applied here too.
> 
> Add another interface to check isIRLevelProfiling.
mask out the variant bits

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:339
@@ -329,2 +338,3 @@
 public:
-  uint64_t getVersion() const { return Index->getVersion(); }
+  /// Return the profile version. Note that is a unmasked version.
+  uint64_t getVersion() const { return GET_VERSION(Index->getVersion()); }
----------------
Remove 'Note that is a unmasked version' -- it is confusing.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:340
@@ +339,3 @@
+  /// Return the profile version. Note that is a unmasked version.
+  uint64_t getVersion() const { return GET_VERSION(Index->getVersion()); }
+  bool isIRLevelProfile() const override {
----------------
No need for GET_VERSION here -- it needs to be applied by the callee

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:342
@@ +341,3 @@
+  bool isIRLevelProfile() const override {
+    return (Index->getVersion() & VARIANT_MASK_IR_PROF) !=0;
+  }
----------------
return Index->isIRLevelProfile();

================
Comment at: include/llvm/ProfileData/InstrProfWriter.h:30
@@ -29,2 +29,3 @@
   typedef SmallDenseMap<uint64_t, InstrProfRecord, 1> ProfilingData;
+  enum ProfKind { PF_None = 0, PF_FE, PF_IRLevel };
 
----------------
PF_None --> PF_Unknown

================
Comment at: lib/ProfileData/InstrProfReader.cpp:117
@@ +116,3 @@
+  bool IsIRInstr = false;
+  while (!Line.is_at_end() &&
+         (Line->empty() || Line->startswith("#") || Line->startswith(":"))) {
----------------
Why not skipping comments and empty lines first:

 // Skip empty lines and comments.
  while (!Line.is_at_end() && (Line->empty() || Line->startswith("#")))
    ++Line;



================
Comment at: lib/ProfileData/InstrProfReader.cpp:119
@@ +118,3 @@
+         (Line->empty() || Line->startswith("#") || Line->startswith(":"))) {
+    if (Line->startswith(":")) {
+      StringRef Str = (Line)->substr(1);
----------------
if (!Line->startsWith(":")) {
   isIRLevelProfile = false;
   return success();
}



================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:132
@@ +131,3 @@
+    if (Writer.setIsIRLevelProfile(IsIRProfile))
+      exitWithError("Merge IR generated profile with Clang generated profile.");
+
----------------
Add a test case for the error.


http://reviews.llvm.org/D15540