[PATCH] D54175: [PGO] context sensitive PGO

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:618
+  // Reserve bit 60-63 for other information purpose.
+  FunctionHash &= 0x0FFFFFFFFFFFFFFF;
+  if (IsCS)
----------------
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.


https://reviews.llvm.org/D54175