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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:486
+  void promoteOnPath(uint32_t SzContextToRemove) {
+    assert(SzContextToRemove <= FullContext.size());
+    FullContext = FullContext.drop_front(SzContextToRemove);
----------------
Add a message.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:560-561
+        return Context1.Callsite < Context2.Callsite;
+      I++;
+      J++;
+    }
----------------
I and J are always the same, then only one var is needed?


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


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1129
+
+  static inline SampleContext getTombstoneKey() { return SampleContext(""); }
+
----------------
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. 


================
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);
----------------
SzContextToRemove -> ContextLevelsToRemove?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108433