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

[Hongtao Yu via Phabricator via llvm-commits]

hoy marked an inline comment as done.
hoy added a subscriber: wlei.
hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:445
   StringRef getNameWithContext() const { return FullContext; }
+  StringRef getInputNameWithContext() const { return InputContext; }
 
----------------
wenlei wrote:
> What about using `getNameWithContext(bool WithBracket = false)`, also change `InputContext` to `FullContextWithBracket`.
That looks better. Thanks.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:325
 
-  assert((RegularProfileCount == 0 || CSProfileCount == 0) &&
+  assert((NonCSProfileCount == 0 || CSProfileCount == 0) &&
          "Cannot have both context-sensitive and regular profile");
----------------
wenlei wrote:
> Use only one counter, and `assert(CSProfileCount == 0 || CSProfileCount == Profiles.size())` ?
Yeah, that's smart.


================
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;
----------------
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();
```


================
Comment at: llvm/test/tools/llvm-profdata/Inputs/csspgo-profile.proftext:1
+[main:3.1 @ _Z5funcBi]:120:19
+ 0: 19
----------------
wenlei wrote:
> Add a function without calling context? We can reuse the test case from our fork?
Sounds good, will port our internal test.


================
Comment at: llvm/test/tools/llvm-profdata/merge-cs-profile.test:1
+# Tests for merge of context-sensitive profile files.
+
----------------
wenlei wrote:
> What about adding a roundtrip conversion test for text and extbin format?
It's actually done by line 4. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95547