[PATCH] D54175: [PGO] context sensitive PGO

[Rong Xu via Phabricator via llvm-commits]

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


================
Comment at: include/llvm/Analysis/ProfileSummaryInfo.h:48
   std::unique_ptr<ProfileSummary> Summary;
-  bool computeSummary();
+  bool computeSummary(bool ForceRecompute = false);
   void computeThresholds();
----------------
vsk wrote:
> xur wrote:
> > vsk wrote:
> > > It doesn't seem necessary to add a flag to computeSummary or to add recomputeSummary to PSI's API.
> > > 
> > > Why not call ProfileSummaryInfo::invalidate when attaching CSPGO data to a Module? invalidate() could be modified to recompute the summary. That keeps the APIs a bit simpler.
> > I agree that change invlidate() would be more clean (and better). My concern was that would change current behavior.
> > invalidate() be called by passmanager. Currently it does nothing, If I change that,  profilesummary could be recomputed many time as most passes will invalidate all the analysis.
> Would it be feasible to update any passes which inadvertently invalidate PSI to preserve it instead?
I found that we don't need to invalidate summary.
Normal PGO-use pass is before per-function optimization  passes
and CS pass of PGO-use is in per-module optimization passes.
They do not share the analyse pass result -- All the results in
per-function optimization pass are discarded and recomputed
again in per-module optimization pass. 

I will revert this change.



================
Comment at: include/llvm/Passes/PassBuilder.h:603
   void addPGOInstrPasses(ModulePassManager &MPM, bool DebugLogging,
-                         OptimizationLevel Level, bool RunProfileGen,
-                         std::string ProfileGenFile,
-                         std::string ProfileUseFile,
+                         OptimizationLevel Level, bool RunProfileGen, bool IsCS,
+                         std::string ProfileGenFile, std::string ProfileUseFile,
----------------
xur wrote:
> vsk wrote:
> > I think it'd be best to consolidate flags here, and replace {bool RunProfileGen, bool IsCS} with {enum InstrumentionKind} (or similar, there must be an enum for this already).
> yes. I agree.
hmm. IsCS is also used by ProfileUse.  Even I combine like this, I still need two flags.


================
Comment at: include/llvm/ProfileData/InstrProfReader.h:148
   bool IsIRLevelProfile = false;
+  bool HasCSIRLevelProfile = false;
 
----------------
xur wrote:
> davidxl wrote:
> > Why? It would be nice to add support in text reader as well.
> Text format is supported (mostly). I say mostly because there is not text string for CS format. But the flat is encoded in header (and in checksum).
Now.  "csir" is supported keyword in proftext format.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:618
+  // Reserve bit 60-63 for other information purpose.
+  FunctionHash &= 0x0FFFFFFFFFFFFFFF;
+  if (IsCS)
----------------
davidxl wrote:
> xur wrote:
> > davidxl wrote:
> > > xur wrote:
> > > > davidxl wrote:
> > > > > This won't work well -- the non-CS FunctionHash may collide with CS hash. How do you differentiate?  
> > > > > 
> > > > > Also doing this for nonCS profiling also breaks the backward compatibility -- old profile with high bits set in hash won't be found anymore. It may also create more hash conflicts.
> > > > That is the reason I bump the version number.  I use the version number to tell if this is old Hash format or new Hash format.
> > > Say there exists an old version profile with the 60-63 bits set for some functions. When this profile is used, the new compiler won't be able to find those functions.
> > > 
> > > Also where is the code that checks the version number?
> > or this we will use one VARIANT bit in header to distinguish. AnnotateAllFunction will check if the VARIANT flag is set when reading CS profiles. 
> > For old index profile, this won't be a problem as this bit will not be set.
> > 
> > This could be a problem for old raw profile. As of now, I did not change instrumentation run-time. So I could only use FuncHash to tell if this regular count or CS count. Setting of VARIANT bit in index profile relies on the new FuncHash scheme.
> > 
> > I can add change to profile runtime and set the VARIANT for raw profile. Then setting the VARIANT bit for index profile will not rely on check of FuncHash. This will fully backward compatible. 
> > 
> > The new FuncHash scheme will be guarded by a check of VARIANT bit. Maybe we don't need bump the profile version.
> > 
> > Right now, It's only in IndexedInstrProfReader::readSummary(). I agree that we probably need to place more checks on this.
> > 
> I think it is better to emit variant bit in raw profile data. Relying on func hashing content sounds fragile. 
> 
> The masking and adding bit in function hash is only needed for CS instrumentation and annotation.  With this, the old profile (non CS) will still be valid.  There is a slight downside that nonCS and CS profile can collide -- but this is very unlikely given all the inlining/cfg transformations that had happened.
Now the raw profile also emit variant.

But I still relying on funchash to tell if the profile for this function is CS or not.

I agree the chance of collision is low.  I think what matter more is the summary computation.  Functions with lots of select instruction will counts as CS profile and used in CS profile summary.  This definitively happens but I'm not sure if matters for performance.

I lean to change the hash unconditionally. For old profiles, we will report a hash-mismatch and the users need to recollect the profile. I think this could happen for other compiler change (like changes before pgo instrumentation).  


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

https://reviews.llvm.org/D54175