[PATCH] D54175: [PGO] context sensitive PGO

[Rong Xu via Phabricator via llvm-commits]

xur marked 3 inline comments as done.
xur added a comment.

Thanks vsk and david for the reviews. I will update the patch to integrate the 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:
> 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.


================
Comment at: include/llvm/IR/Module.h:859
   /// Attach profile summary metadata to this module.
-  void setProfileSummary(Metadata *M);
+  void setProfileSummary(Metadata *M, bool IsCS = false);
 
----------------
vsk wrote:
> I think {set, get}ProfileSummary would be safer & easier to understand if clients had to pass in a non-optional enum specifying which profile data kind they need (maybe use ProfileSummary::Kind for this?). That should make it impossible to mix-and-match CS + the wrong format.
I will change to use an enum parameter (and no default).


================
Comment at: include/llvm/IR/ProfileSummary.h:50
   const Kind PSK;
-  static const char *KindStr[2];
+  static const char *KindStr[3];
   SummaryEntryVector DetailedSummary;
----------------
vsk wrote:
> Does this really need to be in the header? Could we just take the opportunity to delete this declaration, and make the definition static?
I think this is a good idea. the strings are only used in ProfileSummary.cpp.  I will change.


================
Comment at: include/llvm/Passes/PassBuilder.h:47
     assert((RunProfileGen ||
-            !SampleProfileFile.empty() ||
-            !ProfileUseFile.empty() ||
-            SamplePGOSupport) && "Illegal PGOOptions.");
+            // CSProfileGen needs to run under ProfileUse -- For now.
+            (RunCSProfileGen && !ProfileUseFile.empty()) ||
----------------
vsk wrote:
> I think this could use a little more explanation (why would CSProfileGen not need ProfileUse?).
Sorry. This will be deleted. 
I used this to work around an issue with out internal tool.

Thanks for catching this.


================
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,
----------------
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.


================
Comment at: include/llvm/ProfileData/InstrProf.h:902
+  // have context sensitive profile
+  Version6 = 6,
+  // The current version is 6.
----------------
davidxl wrote:
> 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.
OK. no version change. 


================
Comment at: include/llvm/ProfileData/InstrProfReader.h:148
   bool IsIRLevelProfile = false;
+  bool HasCSIRLevelProfile = false;
 
----------------
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).


================
Comment at: include/llvm/ProfileData/InstrProfReader.h:473
   /// Return the maximum of all known function counts.
-  uint64_t getMaximumFunctionCount() { return Summary->getMaxFunctionCount(); }
+  uint64_t getMaximumFunctionCount(bool IsCS = false) {
+    if (IsCS) {
----------------
vsk wrote:
> I'm not sure that the right behavior here should be to default to the non-context sensitive data. Why not either make the bool argument non-optional, or, take the max of the two?
I can change the take bool argument.

Taking max is not a good idea as the client need to know what kind of summary it uses. Mixing these two will cause wrong hot/cold attributes. 


================
Comment at: include/llvm/ProfileData/InstrProfWriter.h:87
+
+    if ((ProfileKind != PF_FE) ^ IsIRLevel)
+      return make_error<InstrProfError>(instrprof_error::unsupported_version);
----------------
vsk wrote:
> Using xor in this context is a bit unconventional. Could you rewrite this using ands/ors to make it easier to understand?
OK. I will change to use and/or


================
Comment at: lib/ProfileData/InstrProfReader.cpp:768
+      IsCS = false;
+    auto &Summary = IsCS ? this->CS_Summary : this->Summary;
+
----------------
vsk wrote:
> vsk wrote:
> > No need to assert on the version, it's checked before readSummary is called.
> Please write out the type of Summary here for clarity.
Do you mean comments?


https://reviews.llvm.org/D54175