[PATCH] D108433: [CSSPGO] split context string - compiler changes
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 24 23:40:30 PDT 2021
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
More information about the llvm-commits
mailing list