[PATCH] D94001: [CSSPGO] Call site prioritized inlining for sample PGO

[Wenlei He via Phabricator via llvm-commits]

wenlei marked an inline comment as done.
wenlei added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/SampleContextTracker.h:100
+  std::vector<const FunctionSamples *>
+  getIndirectCalleeContextSamplesFor(const DILocation *DIL);
   // Query context profile for a given location. The full context
----------------
wmi wrote:
> Do we expect DIL to be the debug location of the indirect call?
Right, discriminator for line-based profile and call site probe id for csspgo profile should differentiate call sites so DIL uniquely identifies a call site. Updated the comment to be explicit.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:43
 ContextTrieNode *
 ContextTrieNode::getChildContext(const LineLocation &CallSite) {
   // CSFDO-TODO: This could be slow, change AllChildContext so we can
----------------
wmi wrote:
> How about getMaxCountChildContext?
Good point, changed to `getHottestChildContext`.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:223-225
+  ContextTrieNode *CallerNode = getContextFor(DIL);
+  LineLocation CallSite = FunctionSamples::getCallSiteIdentifier(DIL);
+  for (auto &It : CallerNode->getAllChildContext()) {
----------------
wmi wrote:
> Is it possible for getContextFor to return nullptr for DIL, and do you need to handle such case?
That shouldn't happen because everything we inlined during sample loader must have a context profile, hence `getContextFor` for any location must have a containing context profile.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:186
+    cl::desc(
+        "The size growth ratio limit for sample profile loader inlining."));
+
----------------
wmi wrote:
> It will only be applied to priority based sample profile loader inlining, right? Same for the description of sample-profile-inline-limit-min/sample-profile-inline-limit-max
Yeah, updated the descriptions. thanks for pointing out.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:368
+  bool operator()(const InlineCandidate &LHS, const InlineCandidate &RHS) {
+    return LHS.CallsiteCount < RHS.CallsiteCount;
+  }
----------------
wmi wrote:
> Considering the case CallsiteCounts are equal, does every InlineCandidate have non-null CalleeSamples? If that is true, FunctionSamples::GUID can be used to make the comparison more stable.
Yeah, every candidate should have non-null CalleeSamples. Added tie breaker using GUID. Thanks for the suggestion.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1389
+  else if (CalleeSamples)
+    CallsiteCount = std::max(CallsiteCount, CalleeSamples->getEntrySamples());
+
----------------
wmi wrote:
> CallsiteCount is 0 definitely before this line, so no need to use std::max. 
> CallsiteCount = CalleeSamples->getEntrySamples();
Good catch, this is a mistaken when taken out the changes for upstream. These are meant to be separate ifs. We found that taking the max works better. Fixed.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1487
+  bool Changed = false;
+  while (!CQueue.empty() && F.getInstructionCount() < SizeLimit) {
+    InlineCandidate Candidate = CQueue.top();
----------------
wmi wrote:
> Several parts in this loop looks quite similar as the counterparts in inlineHotFunctions. Is it possible to make them shared?
Yeah, I thought about that too. I hoisted out part of ICP into tryPromoteIndirectCall to be shared. Looking more at this, I think if we let inlineHotFunctions uses InlineCandidate (with dummy hotness), we should be able to reuse tryInlineCandidate for inlineHotFunctions, and that may enable more shared code for the two loops. What about let me try this with an NFC patch on top of this one, so this patch doesn't change existing inlining code too much? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94001