[PATCH] D54175: [PGO] context sensitive PGO

[Vedant Kumar via Phabricator via llvm-commits]

vsk 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();
----------------
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?


================
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) {
----------------
xur wrote:
> 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. 
Sounds good.


================
Comment at: lib/ProfileData/InstrProfReader.cpp:768
+      IsCS = false;
+    auto &Summary = IsCS ? this->CS_Summary : this->Summary;
+
----------------
xur wrote:
> 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?
I meant 'ProfileSummary &Summary', instead of 'auto &Summary'. I think the coding guidelines suggest writing out the type explicitly when it may not be obvious from the immediate context. This is subjective though, I don't have a strong opinion about this.


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

https://reviews.llvm.org/D54175