[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();
----------------
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.


================
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);
 
----------------
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.


================
Comment at: include/llvm/IR/ProfileSummary.h:50
   const Kind PSK;
-  static const char *KindStr[2];
+  static const char *KindStr[3];
   SummaryEntryVector DetailedSummary;
----------------
Does this really need to be in the header? Could we just take the opportunity to delete this declaration, and make the definition static?


================
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()) ||
----------------
I think this could use a little more explanation (why would CSProfileGen not need ProfileUse?).


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


================
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) {
----------------
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?


================
Comment at: include/llvm/ProfileData/InstrProfReader.h:500
   InstrProfSymtab &getSymtab() override;
-  ProfileSummary &getSummary() { return *(Summary.get()); }
+  ProfileSummary &getSummary(bool IsCS = false) {
+    if (IsCS) {
----------------
I don't think the bool argument here should have a default value. Clients should explicitly pick which kind of summary they need.


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


================
Comment at: lib/ProfileData/InstrProfReader.cpp:768
+      IsCS = false;
+    auto &Summary = IsCS ? this->CS_Summary : this->Summary;
+
----------------
No need to assert on the version, it's checked before readSummary is called.


================
Comment at: lib/ProfileData/InstrProfReader.cpp:768
+      IsCS = false;
+    auto &Summary = IsCS ? this->CS_Summary : this->Summary;
+
----------------
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.


https://reviews.llvm.org/D54175