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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:423
+
+  std::string str(bool OutputLineLocation) const {
+    std::ostringstream OContextStr;
----------------
wenlei wrote:
> 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? 
`ContextFrame` sounds good. How about `SampleContextFrame`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433