[PATCH] D54175: [PGO] context sensitive PGO

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Mostly minor comments inline. But still missing tests. Needs testing via new gold-plugin and opt options. Should also add options to llvm-lto2 to enable testing the LTOBackend path without needing gold or any other specific linker.



================
Comment at: include/llvm/Passes/PassBuilder.h:55
+    // a profile.
+    assert(this->CSAction != CSIRUse || this->Action == IRUse);
+
----------------
Should we also have IRUse when CSAction is CSIRInstr?


================
Comment at: include/llvm/ProfileData/InstrProfReader.h:438
   const unsigned char *readSummary(IndexedInstrProf::ProfVersion Version,
-                                   const unsigned char *Cur);
+                                   const unsigned char *Cur, bool IsCS);
 
----------------
tejohnson wrote:
> Document new parameter here and elsewhere. I assume it is legal to have both a CS and a non-CS summary in this object? In that case, maybe the parameter name should be something like UseCS.
The same change should be done elsewhere in this class (document and change to UseCS). There are a couple other interfaces with the same option.


================
Comment at: include/llvm/ProfileData/InstrProfWriter.h:77
   /// Set the ProfileKind. Report error if mixing FE and IR level profiles.
-  Error setIsIRLevelProfile(bool IsIRLevel) {
+  Error setIsIRLevelProfile(bool IsIRLevel, bool WithCS) {
     if (ProfileKind == PF_Unknown) {
----------------
Document new parameter (there is currently nowhere in the file that indicates CS means context sensitive).


================
Comment at: include/llvm/Transforms/Instrumentation.h:91
 // PGO Instrumention
-ModulePass *createPGOInstrumentationGenLegacyPass();
+ModulePass *createPGOInstrumentationGenLegacyPass(bool IsCS = false);
 ModulePass *
----------------
Document new parameter here and further down in createInstrProfilingLegacyPass (there is currently nowhere in the file that indicates CS means context sensitive).


================
Comment at: lib/Passes/PassBuilder.cpp:523
   // at -Os/Oz.
-  if (!isOptimizingForSize(Level)) {
+  if (!isOptimizingForSize(Level) && !IsCS) {
     InlineParams IP;
----------------
xur wrote:
> tejohnson wrote:
> > Why do we skip size optimizations with CS?
> This is for the early inline. We skip the early inline if for optimization for size. 
> For CS, we will not do early inline either.
Please add comment to that effect.


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:283
   if (OptLevel > 0 && SizeLevel == 0 && !DisablePreInliner &&
-      PGOSampleUse.empty()) {
+      PGOSampleUse.empty() && !IsCS) {
     // Create preinline pass. We construct an InlineParams object and specify
----------------
Similar suggestion to the one I had for new PM (add comment about now not when IsCS).


================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:999
 void InstrProfiling::emitInitialization() {
-  StringRef InstrProfileOutput = Options.InstrProfileOutput;
-
-  if (!InstrProfileOutput.empty()) {
-    // Create variable for profile name.
-    Constant *ProfileNameConst =
-        ConstantDataArray::getString(M->getContext(), InstrProfileOutput, true);
-    GlobalVariable *ProfileNameVar = new GlobalVariable(
-        *M, ProfileNameConst->getType(), true, GlobalValue::WeakAnyLinkage,
-        ProfileNameConst, INSTR_PROF_QUOTE(INSTR_PROF_PROFILE_NAME_VAR));
-    if (TT.supportsCOMDAT()) {
-      ProfileNameVar->setLinkage(GlobalValue::ExternalLinkage);
-      ProfileNameVar->setComdat(M->getOrInsertComdat(
-          StringRef(INSTR_PROF_QUOTE(INSTR_PROF_PROFILE_NAME_VAR))));
-    }
+  // Don't create ProfileName variable for context-sensitive instrumetation
+  // lowering: This lowering is after LTO/ThinLTO linking. Pass
----------------
typo in instrumentation (missing n in the middle).


================
Comment at: tools/opt/opt.cpp:290
 
+cl::opt<CSPGOKind> CSPGOKindFlag(
+    "cspgo-kind", cl::init(NoCSPGO), cl::Hidden,
----------------
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.


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

https://reviews.llvm.org/D54175