[PATCH] D95547: [CSSPGO] Support of CS profiles in extended binary format.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:445
   StringRef getNameWithContext() const { return FullContext; }
+  StringRef getInputNameWithContext() const { return InputContext; }
 
----------------
wlei wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > What about using `getNameWithContext(bool WithBracket = false)`, also change `InputContext` to `FullContextWithBracket`.
> > > That looks better. Thanks.
> > missed the rename of InputContext to FullContextWithBracket?
> I didn't see any where in this patch add the bracket. Do we need to add it in this patch? because the input `ContextStr` actually doesn't have the bracket, the bracket in the regression test is printed by the text format which added the bracket in the end, but not for extended binary.
> 
This patch doesn't add brackets. It relies on bracketed input, like a real text profile, or in-memory profiles fed by llvm-profgen. Currently when llvm-profgen creates a samplecontext , it doesn't add brackets. Instead, it sets `FunctionSamples::ProfileIsCS` explicitly. However, the context should have brackets as well so that the the contextness can be inferred automatically without setting   `ProfileIsCS`.


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:150
   uint64_t Offset = OutputStream->tell();
-  StringRef Name = S.getName();
+  StringRef Name = S.getInputNameWithContext();
   FuncOffsetTable[Name] = Offset - SecLBRProfileStart;
----------------
wlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > If we keep StringRef for context with bracket, we can use it for text profile writer as well. 
> > > 
> > > As we discussed off patch, the context provided by llvm-profgen may not have [], and it's probably better to fix that to make it consistent. 
> > Exactly. The text writer for now has to emit the brackets explicitly like below. @wlei this can be fixed by including brackets in the final context string during profile generation.
> > 
> > 
> > ```
> > std::error_code SampleProfileWriterText::writeSample(const FunctionSamples &S) {
> >   auto &OS = *OutputStream;
> >   if (FunctionSamples::ProfileIsCS)
> >     OS << "[" << S.getNameWithContext() << "]:" << S.getTotalSamples();
> >   else
> >     OS << S.getName() << ":" << S.getTotalSamples();
> > ```
> Got it! So I will fix like the code below:
> 
> ```
>   if (FunctionSamples::ProfileIsCS)
>     OS << S.getNameWithContext()  << S.getTotalSamples();
>   else
>     OS << S.getName() << ":" << S.getTotalSamples();
> ```
Thanks. Ideally it should be just `OS << S.getNameWithContext(true)  << S.getTotalSamples();` unconditionally once brackets are added to contexts on the llvm-profgen side.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95547