[PATCH] D124632: [CSSPGO] Turn on priority inlining for probe-only profile

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2030
+      if (!UsePreInlinerDecision.getNumOccurrences())
+        UsePreInlinerDecision = true;
+    } else if (!Reader->profileIsCS()) {
----------------
Actually when we use preinline decision should we also ignore size limits? We don't have to do it here, but perhaps something worth trying. 

When preinliner is well tuned, I think size limit only makes sense if we have flat cs profile without preinline. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2035-2038
+      if (!ProfileInlineLimitMin.getNumOccurrences())
+        ProfileInlineLimitMin = std::numeric_limits<unsigned>::max();
+      if (!ProfileInlineLimitMax.getNumOccurrences())
+        ProfileInlineLimitMax = std::numeric_limits<unsigned>::max();
----------------
hoy wrote:
> wenlei wrote:
> > Do we also need to clear ProfileInlineGrowthLimit here? 
> That one is used as a multiplier. Giving it a large value may cause overflow, so not sure what is a good value to give.
> 
>      unsigned SizeLimit = F.getInstructionCount() * ProfileInlineGrowthLimit;
>      SizeLimit = std::min(SizeLimit, (unsigned)ProfileInlineLimitMax);
>      SizeLimit = std::max(SizeLimit, (unsigned)ProfileInlineLimitMin);
Ok, with that SizeLimit calculation, as long as we have ProfileInlineLimitMin as int max, the SizeLimit will be int max. We should be good then.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2043
+  if (FunctionSamples::ProfileIsCS) {
+    ProfileIsCS = Reader->profileIsCS();
+    // Tracker for profiles under different context
----------------
Is it actually possible that FunctionSamples::ProfileIsCS is true but Reader->profileIsCS() is false? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124632