[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:
> hoy wrote:
> > 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
> The code you linked (shouldInlineCandidate) is about how we evaluate a candidate, but when size limit is hit, we stop evaluating new candidate: https://github.com/llvm/llvm-project/blob/fc7573f29c79a4bbcfc0d6d001b7c7ae9f1344c7/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1451
Oh I see. Good point, yeah, the size limits should be ignored. I'll give it an evaluation. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2043
+  if (FunctionSamples::ProfileIsCS) {
+    ProfileIsCS = Reader->profileIsCS();
+    // Tracker for profiles under different context
----------------
wenlei wrote:
> hoy wrote:
> > 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.
> If it's not possible, we can do 
> 
> ```ProfileIsCS = true;```
> 
> instead of 
> 
> ```ProfileIsCS = Reader->profileIsCS();```
It should be possible. Let me make `ProfileIsCS = true;` for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124632