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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:454
     assert(!ContextStr.empty());
+    FullContextWithBracket = ContextStr;
     // Note that `[]` wrapped input indicates a full context string, otherwise
----------------
Should we merge it to the condition in line 462. because as its name "WithBracket", it should always have bracket or we can add a check for the `HasContext`.
```
if (HasContext) {
  FullContextWithBracket = ContextStr;
}
```


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:445
   StringRef getNameWithContext() const { return FullContext; }
+  StringRef getInputNameWithContext() const { return InputContext; }
 
----------------
hoy wrote:
> 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`.
Thanks for your explanation, that makes a lot of sense.

So we need to change the llvm-profgen side, to add the brackets for the input `ContextStr`, then we don't need to relay on the text format, also that's the right way for extended format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95547