[PATCH] D54175: [PGO] context sensitive PGO

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

Please also add a profile dumping/reading round trip test with isCS flag (there are existing similar test for llvm-profdata)

text-format --> index-format-> text-format  : diff text-format



================
Comment at: include/llvm/ProfileData/InstrProfWriter.h:91
+    // Promote to PF_IRLevelWithCS if WithCS is true;
+    if (ProfileKind == PF_IRLevel && WithCS)
+      ProfileKind = PF_IRLevelWithCS;
----------------
What is this (promotion)'s use case?


================
Comment at: include/llvm/Transforms/Instrumentation/PGOInstrumentation.h:31
 /// The instrumentation (profile-instr-gen) pass for IR based PGO.
+class PGOInstrumentationGenCreateVar
+    : public PassInfoMixin<PGOInstrumentationGenCreateVar> {
----------------
Add documentation for the new class. Also explain why it needs to be created in a pass.


================
Comment at: lib/Analysis/ProfileSummaryInfo.cpp:89
+  // This will actually return PSK_Instr or PSK_Sample summary.
+  SummaryMD = M.getProfileSummary(ProfileSummary::PSK_Instr);
   if (!SummaryMD)
----------------
This is confusing. It is better to just pass 'IsCS' flag to getProfileSummary method.


================
Comment at: lib/ProfileData/InstrProf.cpp:1015
+// Create variable for profile name.
+void createProfileNameVar(Module &M, StringRef InstrProfileOutput) {
+  if (InstrProfileOutput.empty())
----------------
nit: createProfileFileNameVar.

Can this one be split out as a NFC change?


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:909
                                  cl::desc("Details for every function"));
+  cl::opt<bool> ShowCS("showcs", cl::init(false),
+                       cl::desc("Show context sensitive counts"));
----------------
New option needs to be documented in command line guide.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54175/new/

https://reviews.llvm.org/D54175