[PATCH] D108433: [CSSPGO] split context string - compiler changes

[Hongtao Yu via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:564
+
+    return (FullContext.size() - I) < (That.FullContext.size() - J);
+  }
----------------
wmi wrote:
> If I and J are always the same, we can just compare "FullContext.size() < That.FullContext.size()". 
Good catch!


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1129
+
+  static inline SampleContext getTombstoneKey() { return SampleContext(""); }
+
----------------
wmi wrote:
> Maybe using a non-empty string which can never be used as function name. It is easy to have a name being "" because of programming mistake. 
Sounds good. Using "@" instead.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:67
     const LineLocation &CallSite, ContextTrieNode &&NodeToMove,
-    StringRef ContextStrToRemove, bool DeleteNode) {
+    uint32_t SzContextToRemove, bool DeleteNode) {
   uint32_t Hash = nodeHash(NodeToMove.getFuncName(), CallSite);
----------------
wmi wrote:
> SzContextToRemove -> ContextLevelsToRemove?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433