[PATCH] D54175: [PGO] context sensitive PGO

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: include/llvm/ProfileData/InstrProf.h:777
+
+  static bool hasCSFlaginHash(uint64_t FuncHash) {
+    return ((FuncHash >> CS_FLAG_IN_FUNC_HASH) & 1);
----------------
nit:   FlaginHash --> FlagInHash


================
Comment at: include/llvm/ProfileData/InstrProf.h:780
+  }
+  static void setCSFlaginHash(uint64_t &FuncHash) {
+    FuncHash |= ((uint64_t)1 << CS_FLAG_IN_FUNC_HASH);
----------------
FlaginHash -> FlagInHash.


================
Comment at: include/llvm/ProfileData/InstrProf.h:902
+  // have context sensitive profile
+  Version6 = 6,
+  // The current version is 6.
----------------
There is probably no need to bump the version number if there is a new variant bit set in the profile data. The format of existing variant do not change.


================
Comment at: include/llvm/ProfileData/InstrProfReader.h:148
   bool IsIRLevelProfile = false;
+  bool HasCSIRLevelProfile = false;
 
----------------
Why? It would be nice to add support in text reader as well.


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


https://reviews.llvm.org/D54175