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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:421
   // Enable CS and pseudo probe functionalities in SampleProf
   FunctionSamples::ProfileIsCS = true;
   FunctionSamples::ProfileIsProbeBased = true;
----------------
hoy wrote:
> This should not be needed either. Perhaps in the future we'll also have the profile writer automatically detect pseudo-probe-based profiles and set `FunctionSamples::ProfileIsProbeBased`. 
How about in `SampleProfWriter` we use the `checksum` field to detect it's a probe-based profile then set the FunctionSamples::ProfileIsProbeBased?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:70
     // Enable context-sensitive functionalities in SampleProf
     FunctionSamples::ProfileIsCS = true;
     for (const auto &BI : BinarySampleCounters) {
----------------
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`?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95929