[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:
> > 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`?
Sounds good too, though it may look too similar to `SampleContextFrames`, which can cause confusion. But either way is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433