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

[Nathan Slingerland via llvm-commits]

slingn added inline comments.

================
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) {
----------------
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().

================
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.
----------------
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.

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

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:668
@@ -667,1 +667,3 @@
 
+// Create a COMDAT variable IR_LEVEL_PROF_VARNAME to let runtime
+// aware this is an ir_level profile so it can set the version flag.
----------------
minor: 'to let runtime' -> 'to make the runtime'

================
Comment at: test/Transforms/PGOProfile/Inputs/branch1.proftext:1
@@ +1,2 @@
+# :1 is the flat to indicate this is IR level profile.
+:1
----------------
typo: flat -> flag

A few more of the same in the other *.proftext files.


http://reviews.llvm.org/D15540