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

[Hongtao Yu via Phabricator via llvm-commits]

hoy marked 2 inline comments as done.
hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:403
+// Represent a callsite with caller function name and line location
+struct SampleCallSiteType {
+  StringRef CallerName;
----------------
wenlei wrote:
> 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.
`CallSiteLocation` sounds good.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:420
+
+  bool hasLineLocation() const { return Callsite.LineOffset; }
+  void dropLineLocation() { Callsite = LineLocation(0, 0); }
----------------
wenlei wrote:
> LineOffset 0 can be a valid call site location given this is offset not absolute line number. 
Good catch. Switched to using INT32_MAX as an invalid line offset.


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


================
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]`
----------------
wenlei wrote:
> 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. 
The new context representation should be consistent with the old one for CS profile, i.e, it can contain a leaf frame only. The sample reader creates such leaf-only context. For non-CS profile, the context string will be empty.

Excluding leaf frame from `FullContext` should also work and it is cleaner by design. But the current way is more natural to implement, for example the `IsPrefixOf` operation, and some operations in llvm-profgen that appends the leaf probe into an existing calling context. Also the constructor takes in an arrayRef of the underlying vector, which, if separated, will need some special handling in the reader.


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


================
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++) {
----------------
wenlei wrote:
> Use std::ostringstream to help repeated string append?
Done.


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


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:519
+  /// Set the name of the function.
+  void setName(StringRef FunctionName) { Name = FunctionName; }
+
----------------
wenlei wrote:
> 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?
Assert added. This supposed to use by non-CS case.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:525
+    Name = Context.back().CallerName;
+    State = CState;
+  }
----------------
wenlei wrote:
> 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).
Sounds good, assert added.


================
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)
----------------
wenlei wrote:
> 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)?
The API can be used to compare `A:1 @ B` with `A:1 @ B:2 @ C`, where we have to drop the line number of `B:2`.

The location of leaf frame was (0,0) previously, now I'm changing it to (-1,0). But the `that` context can still have a valid line number like `B:2` where we have to drop the line number for comparision.


================
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
----------------
wenlei wrote:
> Clarify in comment whether this is empty for non-CS profile.
Done.


================
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 {
----------------
wenlei wrote:
> This is a comparer only as it doesn't provide sorting directly. So ProfileComparer? 
Sounds good.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:307-309
+      ContextTrieNode *FromNode = getContextFor(Context);
+      if (FromNode == Node)
+        continue;
----------------
wenlei wrote:
> Curious what triggered this order change?
The context of an inlined or merged node may be out-of-sync with its tree path so `getContextFor` will AV in that case. This can happen when functions on a tree path are not processed in top-down order, due to recursions.


================
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);
----------------
wenlei wrote:
> hoy wrote:
> > wmi wrote:
> > > SzContextToRemove -> ContextLevelsToRemove?
> > Sounds good.
> One step further, ContextFramesToRemove?
Renamed to `ContextFramesToRemove`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433