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

Nathan Slingerland via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 21 11:59:29 PST 2015

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.
typo: flat -> flag

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


More information about the llvm-commits mailing list