[PATCH] D89723: [CSSPGO][llvm-profgen]Context-sensitive profile data generation

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:164
+  }
+  FunctionProfile.addTotalSamples(Count);
+}
----------------
This would not be consistent with the definition of total samples. I think we should only add the portion that was added to body samples.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:208
+
+void CSProfileGenerator::populateFunctionHeadSamples() {
+  for (const auto &Item : Reader->getBranchCounts()) {
----------------
This now actually does more than head samples as call target value sample is handled here too. Perhaps `populateFunctionBoundarySamples` ? 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:221
+      FunctionSamples &FunctionProfile =
+          getFunctionProfileForContext(ContextId);
+
----------------
Same as line 195, this could be hoisted out of the loop?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:239
+      if (ProfileMap.find(ContextForTarget) != ProfileMap.end()) {
+        sampleprof::FunctionSamples &TargetProfile =
+            ProfileMap[ContextForTarget];
----------------
explicit namespace is not needed here. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:263
+
+void CSProfileGenerator::populateFunctionValueSamples() {
+  for (const auto &Item : ProfileMap) {
----------------
This is now more than just value samples. The difference is this one us context to infer missing samples, but others use range and branch to populate samples directly. So name it `populateInferredFunctionSamples`?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:278
+    StringRef NameWithContext;
+    SourceLocation &&CallerLeafInfo =
+        getCallerConext(CalleeContext, NameWithContext);
----------------
nit: name it `CallerLeafFrameLoc`?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:282
+    // It's possible that we haven't seen any sample directly in the caller,
+    // in which case caller_profile will not exist. But we can't modify
+    // ProfileMap while iterating it.
----------------
`caller_profile` -> `CallerProfile`


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:44
+  void update(uint64_t Addr);
+  InstructionPointer &operator++() {
+    advance();
----------------
Is operator++ and operator-- used? these two are duplicated with advance/backward, and we only need to keep one set?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:147
+
+  std::string getFuncFromStartAddr(uint64_t StartAddr) {
+    uint64_t Offset = virtualAddrToOffset(StartAddr);
----------------
return a const ref, or StringRef since FuncStartAddrMap owns the string?


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:37-42
   PerfReader Reader;
   Reader.run();
 
+  std::unique_ptr<ProfileGenerator> Generator =
+      ProfileGenerator::create(&Reader);
+  Generator->generateProfile();
----------------
If we let ProfileGenerator be the driver, I think we should also let ProfileGenerator initiate the perf loading (line 38); otherwise if we intend to decouple them, and let PerfReader read profile outside of ProfileGenerator, then it's better only pass the loaded profile to ProfileGenerator for cleaner separation. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723