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

[Rong Xu via llvm-commits]

xur marked 5 inline comments as done.

================
Comment at: include/llvm/ProfileData/InstrProfWriter.h:57
@@ +56,3 @@
+  /// Set the ProfileKind. Report error if mixing FE and IR level profiles.
+  std::error_code setIsIRLevelProfile(bool IsIRLevel) {
+    if (ProfileKind == PF_Unknown) {
----------------
slingn wrote:
> Is it possible to determine ProfileKind (and IsIRLevel) at construction time? If it is, that seems like it would be less error prone than inferring it from IsIRLevel / leaving as PF_Unknown if setIsIRLevelProfile().
in current profile merge, we generate the writer before reading the profile. So it's difficult to set the ProfileKind in the construct. We could sink the code of creation of the writer into the loop that reading the profiles (after reading the first one).  But I'm not sure that coding style is good.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:113
@@ +112,3 @@
+// Read the profile variant flag from the header: ":0" means this is a FE
+// generated profile. ":1" means this is an IR level profile. Other strings
+// with a leading ':' will be reported an error format.
----------------
slingn wrote:
> If the text format is intended to be mostly human readable it would be nice to use something like ':irlevel' rather than a number since that requires referring to the code.
The advantage of integer is that we can encode other profile variant into one number. As of now, this is the only variant but it's likely we will have more. Also note that usually the text format is generated by llvm-profdata in which case, we do have a comment to explain the the meaning of ":1". 

================
Comment at: lib/ProfileData/InstrProfReader.cpp:130
@@ +129,3 @@
+      return instrprof_error::bad_header;
+  }
+  ++Line;
----------------
slingn wrote:
> Does this handle the case where V could not be parsed as an integer - i.e. getAsInteger() returns true to indicate an error?
Good catch. Fixed in the new patch.


http://reviews.llvm.org/D15540