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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:423
+
+  std::string str(bool OutputLineLocation) const {
+    std::ostringstream OContextStr;
----------------
hoy wrote:
> wenlei wrote:
> > I think as long as we don't use 0 line offset to indicate no line info, it's fine. 
> > 
> > Actually if we use CallSiteLocation to represent leaf, this really isn't a call site (call site always has a line location), but rather a frame. The comment is also inaccurate: "Represent a callsite with caller function name and line location".
> > 
> > We have `typedef std::pair<std::string, LineLocation> FrameLocation;` in llvm-profgen, once we have string buffers owning the underlying strings, that FrameLocation can be merged too.  
> > 
> > Btw, I hope we can unify string converter to be `toString`. Right now we have CallSiteLocation::str(), SampleContext::getContextString() and SampleContext::toString. 
> > 
> > SampleContext::toString properly dispatches for CS and non-CS case, but SampleContext::getContextString() doesn't. How about merge the two and let toString take a boolean? 
> Right `CallSiteLocation` is really for a callsite, we just take advantage of it to represent a leaf frame in `SampleContext`, and we intentionally ignore the line location for leaf frame. This isn't ideal, but easier to implement. Alternatively, the `LineLocation` field can be made an `Optional` field, but that adds the complexity most of the time we don't need.
> 
> The `FrameLocation` type is being replaced by D108437.
> 
> I'm renaming `CallSiteLocation::str()` as `toString`. `SampleContext::getContextString()` is a static function that works on an `ArrayRef` object. This is mostly for the sake of intermediate contexts (without leaf probe) in llvm-profgen. `SampleContext::toString` is not supposed to work with intermediate contexts. I can move `getContextString` out of `SampleContext` if it is  confusing here.
> 
> 
Ok, keeping static function getContextString is fine. 

Given we have SampleContextFrames, SampleContextFrameVector, perhaps this CallSiteLocation should be renamed to ContextFrame to reflect what it really is? 


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:307-309
+      ContextTrieNode *FromNode = getContextFor(Context);
+      if (FromNode == Node)
+        continue;
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > 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.
> > Could you share a concrete example for such case? I was expecting context to be in-sync with its position in trie, because when we promote a node in trie, we also adjust its context accordingly. Does it also happen before this change? 
> Unfortunately I don't seem to find that extreme case after adjusting the context promotion order, i.e, with a comparer. On the other hand, if we are not going to do anything for an inlined or merged context, we can bail out earlier without taking time to find its context node?
Avoid context trie look up sounds good. Just wanted to make sure we don't have lurking issues as I don't see how they can be out of sync - it could be some other issues. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433