[PATCH] D54175: [PGO] context sensitive PGO

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
Comment at: include/llvm/Passes/PassBuilder.h:55
+    // a profile.
+    assert(this->CSAction != CSIRUse || this->Action == IRUse);
+
----------------
tejohnson wrote:
> Should we also have IRUse when CSAction is CSIRInstr?
CSIRInstr can run without IRUSE. CSIRUse needs to IRUse (the only reason is they share the profile).


================
Comment at: include/llvm/ProfileData/InstrProfWriter.h:91
+    // Promote to PF_IRLevelWithCS if WithCS is true;
+    if (ProfileKind == PF_IRLevel && WithCS)
+      ProfileKind = PF_IRLevelWithCS;
----------------
davidxl wrote:
> What is this (promotion)'s use case?
when merging a non-cs profile with cs profile, the result is cs profile.


================
Comment at: lib/ProfileData/InstrProf.cpp:1015
+// Create variable for profile name.
+void createProfileNameVar(Module &M, StringRef InstrProfileOutput) {
+  if (InstrProfileOutput.empty())
----------------
davidxl wrote:
> nit: createProfileFileNameVar.
> 
> Can this one be split out as a NFC change?
split the change to nfc r353230.


================
Comment at: tools/opt/opt.cpp:290
 
+cl::opt<CSPGOKind> CSPGOKindFlag(
+    "cspgo-kind", cl::init(NoCSPGO), cl::Hidden,
----------------
tejohnson wrote:
> The reason I suggested moving this here was to use the same set of options to set up CSPGO for the old PM. But it doesn't seem like it is being set up when invoked via opt. I'd expect something down in AddOptimizationPasses like where it sets up regular IR PGO PassManagerBuilder options.
OK. Got it. 


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

https://reviews.llvm.org/D54175