[PATCH] D90125: [CSSPGO] Infrastructure for context-sensitive Sample PGO and Inlining

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:347
+// State of context associated with FunctionSamples
+enum ContextState {
+  UnknownContext = 0x0,   // Profile without context
----------------
Since these states are not mutually exclusive, perhaps name it ContextStateMask?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:364
+
+  void promoteOnPath(StringRef ContextStrToRemove) {
+    assert(FullContext.startswith(ContextStrToRemove));
----------------
document this method.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:377
+
+  static void DecodeContextString(StringRef ContextStr, StringRef &FName,
+                                  LineLocation &LineLoc) {
----------------
Document the context string format here.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:409
+private:
+  void setContext(StringRef ContextStr, ContextState CState) {
+    assert(!ContextStr.empty());
----------------
what is the input format? document it here.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:504
+    if (ret == BodySamples.end()) {
+      // For CSFDO, in order to conserve profile size, we no longer write out
+      // locations profile for those not hit during training, so we need to
----------------
CSFDO ==> CSSPGO


================
Comment at: llvm/include/llvm/Transforms/IPO/SampleContextTracker.h:94
+  SampleContextTracker(StringMap<FunctionSamples> &Profiles);
+  FunctionSamples *getCalleeContextSamplesFor(const CallBase &Inst,
+                                              StringRef CalleeName);
----------------
Document the public APIs.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:225
       }
-      Profiles[FName] = FunctionSamples();
-      FunctionSamples &FProfile = Profiles[FName];
-      FProfile.setName(FName);
+      SampleContext FContext(FName);
+      if (FContext.hasContext())
----------------
so FName is also the context String?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90125