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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


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


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:757
+  /// Return the input string of function name with context.
+  StringRef getInputNameWithContext() const {
+    return FunctionSamples::ProfileIsCS ? Context.getInputNameWithContext()
----------------
same here, merge with `getNameWithContext(bool WithBracket=false)`?


================
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");
----------------
Use only one counter, and `assert(CSProfileCount == 0 || CSProfileCount == Profiles.size())` ?


================
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;
----------------
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. 


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


================
Comment at: llvm/test/tools/llvm-profdata/merge-cs-profile.test:1
+# Tests for merge of context-sensitive profile files.
+
----------------
What about adding a roundtrip conversion test for text and extbin format?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95547