[PATCH] D108433: [CSSPGO] split context string - compiler changes

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Thanks for working on split context representation. This really makes context first class citizen in the framework, and the speed up it brought us is also very nice.



================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:403
+// Represent a callsite with caller function name and line location
+struct SampleCallSiteType {
+  StringRef CallerName;
----------------
nit: having the word `Type` in type name does not add value, and the class also doesn't have anything specific for sample. Suggest we call it CallSiteLocation.

There're a few other others with `Type` in its name too.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:420
+
+  bool hasLineLocation() const { return Callsite.LineOffset; }
+  void dropLineLocation() { Callsite = LineLocation(0, 0); }
----------------
LineOffset 0 can be a valid call site location given this is offset not absolute line number. 


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:440-441
+
+using SampleContextStorageType = SmallVector<SampleCallSiteType, 10>;
+using SampleContextRefType = ArrayRef<SampleCallSiteType>;
+
----------------
SampleContextRefType makes people think of SampleContext&, how about SampleContextStorageType -> SampleContextFrameVector, SampleContextRefType -> SampleContextFrames?

Accordingly SampleContext::getFullContext->SampleContext::getContextFrames.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:449-457
 // Sample context for FunctionSamples. It consists of the calling context,
 // the function name and context state. Internally sample context is represented
 // using StringRef, which is also the input for constructing a `SampleContext`.
 // It can accept and represent both full context string as well as context-less
 // function name.
 // Example of full context string (note the wrapping `[]`):
 //    `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]`
----------------
This needs update. 

One thing I was looking for a clarification from comments somewhere but didn't find it: whether context-less profile would have a non-empty FullContext which only contains leaf? From the code it seems answer is no? But then this is not consistent since FullContext of CS profile contains leaf frame.

Or would it be possible to remove leaf frame in FullContext for CS profile too? leaf doesn't need call site location either. 


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:494
+  StringRef getName() const { return Name; }
   StringRef getNameWithoutContext() const { return Name; }
+  SampleContextRefType getFullContext() const { return FullContext; }
----------------
Can we remove this and replace all its call with getName?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:498
+  static std::string getContextString(SampleContextRefType Context) {
+    std::string ContextStr;
+    for (uint32_t I = 0; I < Context.size(); I++) {
----------------
Use std::ostringstream to help repeated string append?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:508
+
+  std::string getContextString() const {
+    if (!hasContext())
----------------
Seems `std::string toString() const` is the conventional name for string converter. 


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:519
+  /// Set the name of the function.
+  void setName(StringRef FunctionName) { Name = FunctionName; }
+
----------------
Sanity check/assert to make sure the new name does not conflict leaf frame in FullContext? Or is this only allowed for non-CS case?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:525
+    Name = Context.back().CallerName;
+    State = CState;
+  }
----------------
Should we assert that CState is not unknown? i.e. even if Context only has one frame, this is still considered context profile (something like [foo] before, as opposed to foo).


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:575-582
+    SampleCallSiteType ThisLeaf = ThisContext.back();
+    SampleCallSiteType ThatLeaf = ThatContext.back();
+    if (!ThisLeaf.hasLineLocation()) {
+      ThisLeaf.dropLineLocation();
+      ThatLeaf.dropLineLocation();
     }
+    if (ThisLeaf != ThatLeaf)
----------------
This sequence of copy, edit and compare could be simplified?

```
if (ThisContext.back().CallerName != ThatContext.back().CallerName)
  return false;
```

Even better, would it be possible to canonicalize context, so location of leaf frame is always (0, 0)?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:591
+  // Full context including calling context and leaf function name
+  SampleContextRefType FullContext;
   // State of the associated sample profile
----------------
Clarify in comment whether this is empty for non-CS profile.


================
Comment at: llvm/include/llvm/Transforms/IPO/SampleContextTracker.h:93
 public:
-  using ContextSamplesTy = SmallVector<FunctionSamples *, 16>;
-
-  SampleContextTracker(StringMap<FunctionSamples> &Profiles);
+  struct ProfileSorter {
+    bool operator()(FunctionSamples *A, FunctionSamples *B) const {
----------------
This is a comparer only as it doesn't provide sorting directly. So ProfileComparer? 


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:307-309
+      ContextTrieNode *FromNode = getContextFor(Context);
+      if (FromNode == Node)
+        continue;
----------------
Curious what triggered this order change?


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:67
     const LineLocation &CallSite, ContextTrieNode &&NodeToMove,
-    StringRef ContextStrToRemove, bool DeleteNode) {
+    uint32_t SzContextToRemove, bool DeleteNode) {
   uint32_t Hash = nodeHash(NodeToMove.getFuncName(), CallSite);
----------------
hoy wrote:
> wmi wrote:
> > SzContextToRemove -> ContextLevelsToRemove?
> Sounds good.
One step further, ContextFramesToRemove?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433