[PATCH] D95929: [CSSPGO][llvm-profgen] Add brackets for context id to support extended binary format
Lei Wang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 5 15:14:30 PST 2021
wlei added inline comments.
================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:70
// Enable context-sensitive functionalities in SampleProf
FunctionSamples::ProfileIsCS = true;
for (const auto &BI : BinarySampleCounters) {
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > wlei wrote:
> > > > wlei wrote:
> > > > > hoy wrote:
> > > > > > This should be removed since now every context has brackets.
> > > > > It seems it also depends on this variable, see the code below. I saw your previous patch set it in the `SampleProfReader ` but llvm-profgen only use the `SampleProfWriter`.
> > > > >
> > > > > ```
> > > > > /// Return function name with context.
> > > > > StringRef getNameWithContext(bool WithBracket = false) const {
> > > > > return FunctionSamples::ProfileIsCS
> > > > > ? Context.getNameWithContext(WithBracket)
> > > > > : Name;
> > > > > }
> > > > > ```
> > > > >
> > > > > I can understand we should automatically detect this, so how about to change the `Writer` to add some detection, say if the name has the bracket then set the `ProfileIsCS`?
> > > > >
> > > > >
> > > > Seems there aren't a central place in `Writer` to detect it, the `write` function is override by different format.
> > > > or how about code like this, we ensure the `Context` is always valid for CS profile, right?
> > > > ```
> > > > StringRef getNameWithContext(bool WithBracket = false) const {
> > > > return Context.getNameWithContext().size()?
> > > > Context.getNameWithContext(WithBracket) : Name ;
> > > > ```
> > > Yeah, we might end up doing the detection per writer. That should only be needed for `ProfileIsPseudoProbe` . For now we don't need to set `ProfileIsCS` because the contexts all have brackets and they can just be treated as regular function profiles by the existing writers.
> > I see, I guess you meant `ProfileIsCS` is false by default, so it will use the default `Name` variable like the regular function and the `Name` variable actually has the brackets.
> >
> > Just fixed following this idea by setting the `Name`.
> > ```
> > FProfile.setName(FContext.getNameWithContext(true));
> > ```
> Exactly. Also we can avoid setting `ProfileIsCS` in llvm-profgen now. It's yet ready to do the same for `ProfileIsProbeBased` since work will be needed on the profile writer side.
Yeah, have removed the `ProfileIsCS`, see the latest diff.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95929/new/
https://reviews.llvm.org/D95929
More information about the llvm-commits
mailing list