[PATCH] D95929: [CSSPGO][llvm-profgen] Add brackets for context id to support extended binary format

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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) {
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95929