[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()) {
----------------
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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2043
+  if (FunctionSamples::ProfileIsCS) {
+    ProfileIsCS = Reader->profileIsCS();
+    // Tracker for profiles under different context
----------------
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();```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124632