[PATCH] D54175: [PGO] context sensitive PGO

[Rong Xu via Phabricator via llvm-commits]

xur marked 41 inline comments as done.
xur added inline comments.


================
Comment at: cmake/modules/HandleLLVMOptions.cmake:816
 
+# Nedd to pass -fprofile-instr-use to backend compiler after ld-plugin.
+# This is for context-sensitive PGO compilation.
----------------
tejohnson wrote:
> s/Nedd/Need
> 
> Also, I don't understand how the comment about needing to pass it to the backend compiler after ld-plugin relates to the handling below.
Fixed the typo.

I think currently -profile-instr-use flag is only  added to CMAKE_CXX_FLAGS and CMAKE_C_FLAGS. I need them to pass to linker and shared linker also. Since there is no flag for CSIR use build. I have to pass them unconditionally.  For regular PGO, the flag is void operation.

I'll remove the line of CMAKE_CXX_FLAGS and CMAKE_C_FLAGS


================
Comment at: cmake/modules/HandleLLVMOptions.cmake:826
+
+option(LLVM_USE_NEWPM "Build LLVM using experimental new pass manager" Off)
+mark_as_advanced(LLVM_USE_NEWPM)
----------------
tejohnson wrote:
> This part seems unrelated - probably split into a different patch?
Moved the change to 
https://reviews.llvm.org/D57068


================
Comment at: include/llvm/Passes/PassBuilder.h:45
+    // We do the following checks:
+    // (1) Action == NoAction && CSAction == NOCSAction && SamplePGOSupport,
+    // (2) if Action != NoAction then !ProfileFile.empty(),
----------------
tejohnson wrote:
> This ordering doesn't seem to match the ordering of the checks in the assert below. In any case, rather than have the documentation just duplicate the code (it doesn't really add anything this way), it would be better to split up the following assert into multiple, with more of a plain english explanation of each one.
Make sense. I will change this part.


================
Comment at: lib/Passes/PassBuilder.cpp:523
   // at -Os/Oz.
-  if (!isOptimizingForSize(Level)) {
+  if (!isOptimizingForSize(Level) && !IsCS) {
     InlineParams IP;
----------------
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.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:666
+  // Hash format for context senstive profile. Reserve 4 bits for other
+  // information.
   FunctionHash = (uint64_t)SIVisitor.getNumOfSelectInsts() << 56 |
----------------
tejohnson wrote:
> Probably needs a slight expansion on why that other info shouldn't affect function hash.
This change actually will affect function hash -- if an old profile is passed here, an non-cs record could be recognized as CS profile. But that will most likely result in a CFG hash mismatch where the user will get a warning.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:678
                     << ValueSites[IPVK_IndirectCallTarget].size()
+                    << ", Selects = " << SIVisitor.getNumOfSelectInsts()
                     << ", Hash = " << FunctionHash << "\n";);
----------------
tejohnson wrote:
> why reorder this?
not intentionally. I will revert this change.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1507
+    function_ref<BlockFrequencyInfo *(Function &)> LookupBFI,
+    ProfileSummaryInfo *PSI, bool IsCS) {
   LLVM_DEBUG(dbgs() << "Read in profile counters: ");
----------------
tejohnson wrote:
> I don't see where the new parameter PSI is getting used.
Good catch! This is from my other changes that fixes BB profile count. They should be removed.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1642
 
+  ProfileSummaryInfo *PSI = nullptr;
+  if (IsCS)
----------------
tejohnson wrote:
> Here and below, needs comment about why PSI when IsCS. But see comment in annotateAllFunctions - I don't see it getting used there?
Fixed. PSI is not used here.


================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:716
+      bool FuncIsCS = NamedInstrProfRecord::hasCSFlagInHash(Func.Hash);
+      if (FuncIsCS && !ShowCS)
+        continue;
----------------
tejohnson wrote:
> How about replace these 4 lines with just:
> 
> if (FuncIsCS != ShowCS)
>    continue;
Yes!


================
Comment at: tools/opt/NewPMDriver.cpp:249
 
+  // TODO: handle CSFDO options.
   Optional<PGOOptions> P;
----------------
tejohnson wrote:
> Is this still TODO? I see handling further below.
Yes. we should remove this TODO.


================
Comment at: tools/opt/NewPMDriver.cpp:284
+        if (!P)
+          errs() << "CSInstrUse needs to be together with InstrUse";
+        P->CSAction = PGOOptions::CSIRUse;
----------------
tejohnson wrote:
> As an aside - theoretically someone might want to do CS profiling without regular PGO. Is it worth supporting that combination?
Since both CSprofile and non-CS-profile are merged into one profile. I think it's earlier to assume regular PGO is enabled for CSPGO. If there is a need for support the other way, I will add later.


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

https://reviews.llvm.org/D54175