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

[hoy]

Summary

We have seen that the prioirty inliner delivered on-par performance with the old inliner for probe-only CSSPGO profile, as long as without a size budget. I'm turning on the priority inliner for probe-only profile by default.

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

2035–2038

Do we also need to clear ProfileInlineGrowthLimit here?

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

2035–2038

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);

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

2030

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.

2035–2038

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.

2043

Is it actually possible that FunctionSamples::ProfileIsCS is true but Reader->profileIsCS() is false?

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

2030

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

2043

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.

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

2030

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

2043

If it's not possible, we can do

ProfileIsCS = true;

instead of

ProfileIsCS = Reader->profileIsCS();

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

2030

Oh I see. Good point, yeah, the size limits should be ignored. I'll give it an evaluation.

2043

It should be possible. Let me make ProfileIsCS = true; for now.

[hoy]

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

[wenlei]

Let's evaluate ignoring size limit for preinline case; otherwise lgtm, thanks.

llvm/lib/Transforms/IPO/SampleProfile.cpp

bool SampleProfileLoader::doInitialization(Module &M,
   if (Reader->profileIsCS() || Reader->profileIsPreInlined() ||
       Reader->profileIsProbeBased()) {
     if (!UseIterativeBFIInference.getNumOccurrences())
       UseIterativeBFIInference = true;
     if (!SampleProfileUseProfi.getNumOccurrences())
       SampleProfileUseProfi = true;
     if (!EnableExtTspBlockPlacement.getNumOccurrences())
       EnableExtTspBlockPlacement = true;
-  }
-
-  if (Reader->profileIsCS() || Reader->profileIsPreInlined()) {
-    ProfileIsCS = Reader->profileIsCS();
     // Enable priority-base inliner and size inline by default for CSSPGO.
     if (!ProfileSizeInline.getNumOccurrences())
       ProfileSizeInline = true;
     if (!CallsitePrioritizedInline.getNumOccurrences())
       CallsitePrioritizedInline = true;
-
-    // For CSSPGO, use preinliner decision by default when available.
-    if (!UsePreInlinerDecision.getNumOccurrences())
-      UsePreInlinerDecision = true;
-
     // For CSSPGO, we also allow recursive inline to best use context profile.
     if (!AllowRecursiveInline.getNumOccurrences())
       AllowRecursiveInline = true;
 
-    if (FunctionSamples::ProfileIsCS) {
-      // Tracker for profiles under different context
-      ContextTracker = std::make_unique<SampleContextTracker>(
-          Reader->getProfiles(), &GUIDToFuncNameMap);
-    }
-  }
+    if (Reader->profileIsPreInlined()) {
+      if (!UsePreInlinerDecision.getNumOccurrences())
+        UsePreInlinerDecision = true;
+    } else if (!Reader->profileIsCS()) {
+      // Non-CS profile should be fine without a function size budget for the
+      // inliner since the contexts in the profile are all from inlining in
+      // the prevoius build, thus they are bounded.
+      if (!ProfileInlineLimitMin.getNumOccurrences())
+        ProfileInlineLimitMin = std::numeric_limits<unsigned>::max();
+      if (!ProfileInlineLimitMax.getNumOccurrences())
+        ProfileInlineLimitMax = std::numeric_limits<unsigned>::max();
+    }
+  }
+
+    ProfileIsCS = true;
 
   // Load pseudo probe descriptors for probe-based function samples.
   if (Reader->profileIsProbeBased()) {
     ProbeManager = std::make_unique<PseudoProbeManager>(M);
     if (!ProbeManager->moduleIsProbed(M)) {
       const char *Msg =
           "Pseudo-probe-based profile requires SampleProfileProbePass";
       Ctx.diagnose(DiagnosticInfoSampleProfile(M.getModuleIdentifier(), Msg,