[PATCH] D108435: [CSSPGO] split context string II - reader/writer changes

[Hongtao Yu via Phabricator via llvm-commits]

hoy marked 2 inline comments as done.
hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:135
 //
 // Binary format
 // -------------
----------------
wenlei wrote:
> In this section, we need to add documentation/spec for CSNameTable how context is stored, and how it interacts with NameTable.
CSNameTable is a concept of the extensible binary format, not the basic binary format, like other concepts such as func name offset table, func metadata table. We could comment about CSNameTable in the `SampleProfileReaderExtBinary/SampleProfileWriterExtBinary`, but I didn't find a good centralized place to do that. Seems that descriptions about other sections are scattered around the implementation code.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:507
   /// to their corresponding profiles.
-  StringMap<FunctionSamples> Profiles;
+
+  SampleProfileMap Profiles;
----------------
wenlei wrote:
> nit: remove the blank line
done.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:579
+
+  /// Create a context vector from a give context string and save it in
+  /// CSNameTable.
----------------
wenlei wrote:
> typo: given
fixed.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfWriter.h:138
   std::error_code writeSummary();
-  std::error_code writeNameIdx(StringRef FName, bool IsContextName = false);
+  virtual std::error_code writeNameIdx(const SampleContext &Context);
+  std::error_code writeNameIdx(StringRef FName);
----------------
wenlei wrote:
> Now that the parameter is no longer a string name, rename the function as well, e.g. writeContextIdx? 
> 
> Same for addName(const SampleContext &Context) -> addContext
Sounds good.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:326-327
+        // otherwise it's treated as context-less function name only.
+        auto Context = createContext(FName);
+        FContext.setContext(Context);
         ++CSProfileCount;
----------------
wenlei wrote:
> Even though we only need to deal with string context in profile reader/writer for text profile, it's probably still cleaner to keep all string context related parsing into SampleContext. 
> 
> `createContext` is more like a ctor. I'd prefer keep string decoding, createContext in its original place in SampleContext. That way, we can construct a context from string, SampleContext::setContext remain a private helper too, and the logic here can be simpler, just like before.
> 
> SampleContext does still have getContextString and toString, so it's not really isolated from string representation, might as well keep all string stuff together there for consistency. 
Makes sense. Moved back to SampleContext.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:496
 ErrorOr<StringRef> SampleProfileReaderBinary::readStringFromTable() {
-  auto Idx = readStringIndex(NameTable);
+  auto NTable = getNameTable();
+  assert(NTable && "NTable should not be nullptr");
----------------
wenlei wrote:
> I think `getNameTable` exist as a virtual function only because of the use in sample loader for `profile-sample-accuarate` and `profile-accurate-for-symsinlist`. Here can we still access the NameTable directly without going through virtual call? Same for other places where `NameTable` is directly accessible. For writer, before this change, we indeed only have `NameTable` for `SampleProfileWriterBinary` without virtual getter, and we access it without going through virtual function call there. 
Yes, we can just use `NameTable` directly here. Actually  all changes here are unnecessary. I just undid them.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:638
 
-  auto FName(readStringFromTable());
-  if (std::error_code EC = FName.getError())
+  auto FContext(readNameFromTable(IsContextName));
+  if (std::error_code EC = FContext.getError())
----------------
wenlei wrote:
> nit: this can be confusing, readNameFromTable can mislead people to think FContext is string (or vector of strings) type. the auto also isn't helping. Either spell out the type name, or rename `readNameFromTable` to something like `readSampleContextFromTable`
> 
> 
Fixed by using explicit type.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:800
+      if (std::error_code EC =
+              readFuncProfile(Data, FunctionSamples::ProfileIsCS))
         return EC;
----------------
wenlei wrote:
> Since we have `SampleProfileReader::ProfileIsCS`, within reader it probable makes more sense to use the reader instance flag as opposed to the global FunctionSamples::ProfileIsCS flag. 
> 
> We were not very consistent in past, might be good to clean up as you're expanding the use of ProfileIsCS. 
That's reasonable. Replaced all `FunctionSamples::ProfileIsCS` usage in the reader with the reader `ProfileIsCS` flag.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:828
       // iterating through the ordered names.
-      struct Comparer {
-        // Ignore the closing ']' when ordering context
-        bool operator()(const StringRef &L, const StringRef &R) const {
-          return L.substr(0, L.size() - 1) < R.substr(0, R.size() - 1);
-        }
-      };
-      std::set<StringRef, Comparer> OrderedNames;
+      std::set<SampleContext> OrderedNames;
       for (auto Name : FuncOffsetTable) {
----------------
wenlei wrote:
> OrderedNames -> OrderedContexts
fixed.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:829-846
       for (auto Name : FuncOffsetTable) {
         OrderedNames.insert(Name.first);
       }
 
       // For each function in current module, load all
       // context profiles for the function.
       for (auto NameOffset : FuncOffsetTable) {
----------------
wenlei wrote:
> Which operation here is the most costly and visible for e2e build time? The insert/sort/find or the prefix check? What's the % of time here for both prelink and postlink?
The insert/sort/find operations are the most expansive. For thinlto postlink, they count to 15% to 20% of the whole backend running time. I haven't measured for prelink but given the similarity of prelink and postlink, they should be expansive there too.

I'm actually working on a sample writer change that emits the func offset table in the order of contexts so that we don't need the set operations here. That turns out very effective. Will send out a separate diff for it.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1102
+
+    PNameVec->emplace_back(Context);
+  }
----------------
wenlei wrote:
> How about we emplace an empty slot before going into the inner loop, then we can operate on the vector directly (`PNameVec.back().emplace_back(...)`) and avoid copying temporary `Context` onto `PNameVec`?
Sounds good, that should be faster.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1117
 
-    SampleContext FContext(*FName);
-    bool ProfileInMap = Profiles.count(FContext);
+    bool ProfileInMap = Profiles.count(FContext.get());
 
----------------
wenlei wrote:
> nit: peal/hoist the `get()` so we don't have to call it for every use.
You mean save `FContext.get()` in a temp and use it in the loop? Thought the compiler would do it.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:506
+ErrorOr<SampleContext>
+SampleProfileReaderBinary::readNameFromTable(bool IsContextName) {
+  auto FName(readStringFromTable());
----------------
wenlei wrote:
> hoy wrote:
> > hoy wrote:
> > > wmi wrote:
> > > > This param is unused. Write it as readNameFromTable(bool /* IsContextName */) to make it more explicit. 
> > > Sounds good.
> > Actually I can only do that in the header file for the function declaration. The function definition needs a real name for param.
> Can we avoid this parameter altogether? For one profile, it's either going to be CS or non-CS, so the dispatch can be based on state/member of the reader instead of relying on a param for each invocation. 
Good point. Checking `FunctionSamples::ProfileIsCS` should be enough.


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:249
+  for (auto Context : OrderedContexts) {
+    auto Callsites = Context.getContextFrames();
+    encodeULEB128(Callsites.size(), OS);
----------------
wenlei wrote:
> nit: this contains leaf frame, so it's `Frames` instead of `Callsites`. 
`Frames` sounds good.


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:482
+SampleProfileWriterBinary::writeNameIdx(const SampleContext &Context) {
+  return writeNameIdx(Context.getName());
+}
----------------
wenlei wrote:
> assert !Context.hasContext() ?
Added the assert.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:524
   for (const auto &PD : Reader->getProfiles()) {
-    StringRef FName = PD.getKey();
-    const sampleprof::FunctionSamples &FS = PD.getValue();
-    auto It = InstrProfileMap.find(FName);
+    auto &FName = PD.first;
+    const sampleprof::FunctionSamples &FS = PD.second;
----------------
wenlei wrote:
> I suggest rename these FName (key of the SampleProfileMap) to be FContext accordingly given this is no longer a string map. Same for other places.
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108435