[PATCH] D98590: [CSSPGO] Load context profile for external functions in PreLink and populate ThinLTO import list

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:730
+          // load it repeatedly.
+          OrderedNames.erase(It++);
+        }
----------------
wmi wrote:
> Better use: It = OrderedNames.erase(It);
good point, changed.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:976
+    for (auto &Child : Node->getAllChildContext()) {
+      ContextTrieNode *CalleeNode = &Child.second;
+      CalleeList.push(CalleeNode);
----------------
wmi wrote:
> The hotness of CalleeNode is not checked before it is inserted into CalleeList. Will it add unnecessary imports?
That is an oversight, good catch! The code was partially following findInlinedFunctions, but since there's no recursion, the hotness check on function entry won't apply to lower callees.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98590