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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:52
+void ProfileGenerator::findDisjointRanges(
+    std::map<std::pair<uint64_t, uint64_t>, uint64_t> &DisjointRanges,
+    const std::map<std::pair<uint64_t, uint64_t>, uint64_t> &Ranges) {
----------------
typedef or using an alias like `RangeCountMap` for this type?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:145
+  }
+  FunctionSamples &FunctionProfile = ProfileMap[ContextStr];
+  return FunctionProfile;
----------------
Just `return ProfileMap[ContextStr]`?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:194
+      // Get or create function profile for the range
+      FunctionSamples &FunctionProfile =
+          getFunctionProfileForContext(ContextId);
----------------
Can this be moved out of the loop? The string-based look up is potentially slow.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:199
+        // Recording body sample for this specific context
+        updateBodySamplesforFunctionProfile(IP.Address, FunctionProfile, Binary,
+                                            Count);
----------------
`Binary` can be achieved via `Reader` so no need to pass in as an argument.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:216
+      uint64_t Count = Entry.second;
+      std::string &&TargetName = Binary->getFuncFromStartAddr(Target);
+      if (TargetName.size() == 0)
----------------
@wmi Looks like here is a place we use ELF symbol name as function name. May need to call `getCanonicalFnName` here.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:56
+  // Lookup or create FunctionSamples for the context
+  FunctionSamples &getFunctionProfileForContext(StringRef &ContextId);
+
----------------
This sounds like a method of `CSProfileGenerator`. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:82
+private:
+  // TODO: to fix
+  void updateBodySamplesforFunctionProfile(uint64_t Address,
----------------
Please make this TODO more clear.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:152
+
+  SourceLocation getInlineLeaf(uint64_t Address, bool NameOnly = false) {
+    uint64_t Offset = virtualAddrToOffset(Address);
----------------
Can this just return a reference so that you don't need to use the move semantics when it gets called?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89723