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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2030
+      if (!UsePreInlinerDecision.getNumOccurrences())
+        UsePreInlinerDecision = true;
+    } else if (!Reader->profileIsCS()) {
----------------
wenlei wrote:
> 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. 
I think with preinline decision, the size limits are actually ignored:

https://github.com/llvm/llvm-project/blob/fc7573f29c79a4bbcfc0d6d001b7c7ae9f1344c7/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1396


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2043
+  if (FunctionSamples::ProfileIsCS) {
+    ProfileIsCS = Reader->profileIsCS();
+    // Tracker for profiles under different context
----------------
wenlei wrote:
> Is it actually possible that FunctionSamples::ProfileIsCS is true but Reader->profileIsCS() is false? 
I don't think so. They are defined by same value in the reader:

https://github.com/llvm/llvm-project/blob/d3e5f0ab01c6b885082a1e12a7dfe5e55d340e52/llvm/lib/ProfileData/SampleProfReader.cpp#L367

https://github.com/llvm/llvm-project/blob/d3e5f0ab01c6b885082a1e12a7dfe5e55d340e52/llvm/lib/ProfileData/SampleProfReader.cpp#L657

Actually I think the `ProfileIsCS` field should be removed, and instead using FunctionSamples::ProfileIsCS everywhere. I can make a separate diff to clean this up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124632