[CSSPGO] Fix incorrect probe distribution factor computation in top-down inliner

[wlei]

Summary

We see a regression related to low probe factor(0.01) which prevents some callsites being promoted in ICPPass and later cause the missing inline in CGSCC inliner. The root cause is due to redundant(the second) multiplication of the probe factor and this change try to fix it.

Sum does multiply a factor right after findCallSamples but later when using as the parameter in setProbeDistributionFactor, it multiplies one again.

This change could get ~2% perf back on mcf benchmark. In mcf, previously the corresponding factor is 1 and it's the recent feature introducing the <1 factor then trigger this bug.

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

875

Thanks for the fix. This seems losing the original distribution factor pre

875

nvm, ignore this comment.

1327

Thanks for the fix! I'm wondering if the fix should be made here by scaling down SumOrigin as well. SumOrigin is used to guide ICP and we are checking the scaled count against the original count below (line 1342). This could cause a promotable indirect callsite not promoted after the callsite is duplicated. E.g, an indirect callsite with only one target is duplicated in prelink with even distribution. And in postlink, each of the callsite has a target that counts 50% of the original counts. If the original callsite is supposed to be promoted, once duplicated, it may not get promoted in postlink because of the lower weight (50% vs original 100%) in either place.

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

1327

It's arguable as to what is more desirable, if a call site is duplicated. If we scale down SumOrigin, the ICP logic would be more consistent between when there's duplication vs not. But on the other hand, we might indeed want to penalize duplicated call sites because ICP (and inline) for all of them has larger cost that is not taken into account. I think that's more of matter of tuning, but the current fix in the patch is addressing something clearly wrong. We would need some perf testing if we're making the change of scaling down SumOrigin.

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

1327

Agreed. Numbers are needed to validate a change scaling down SumOrigin. Current change looks good to me.

@wlei Can you please add a test?

[wlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

1327

Thanks for the explanation!

Can you please add a test?

Good point, will try to add a test.

[wlei]

ad test case: manually make a < 1 factor of the promotable call site, after ICP check the discriminator

[hoy]

llvm/test/Transforms/SampleProfile/pseudo-probe-icp-factor.ll

248 ↗

(On Diff #335295)

Thanks for adding the test.

Can you comment here how to interpret the discriminator? Can you also add a check for which call instruction this metadata is attached to? An example could be test/Transforms/SampleProfile/pseudo-probe-profile.ll

[wlei]

Addressing Hongtao's feedback

[hoy]

lgtm, thanks!

llvm/lib/Transforms/IPO/SampleProfile.cpp

875

Nit: may just do a type casting here instead of using the multiplication. The compiler may not optimize it away in different FP models.

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

875

+1 for explicit casting

[wlei]

switch to explicit cast

[wenlei]

lgtm, thanks.

[wenlei]

Going to pushed this for Lei (who's on leave for the week), so it's available in private fork too.

llvm/lib/Transforms/IPO/SampleProfile.cpp

if (!R->getValue()->isDeclaration() && R->getValue()->getSubprogram() &&
     if (DI) {
       Sum -= Candidate.CallsiteCount;
       // Prorate the indirect callsite distribution.
       // Do not update the promoted direct callsite distribution at this
       // point since the original distribution combined with the callee
       // profile will be used to prorate callsites from the callee if
       // inlined. Once not inlined, the direct callsite distribution should
       // be prorated so that the it will reflect the real callsite counts.
-      setProbeDistributionFactor(CI, Candidate.CallsiteDistribution * Sum /
-                                         SumOrigin);
+      setProbeDistributionFactor(CI, 1.0 * Sum / SumOrigin);
       Candidate.CallInstr = DI;
       if (isa<CallInst>(DI) || isa<InvokeInst>(DI)) {
         bool Inlined = tryInlineCandidate(Candidate, InlinedCallSite);
         if (!Inlined) {
           // Prorate the direct callsite distribution so that it reflects real
           // callsite counts.
-          setProbeDistributionFactor(*DI, Candidate.CallsiteDistribution *
-                                              Candidate.CallsiteCount /
-                                              SumOrigin);
+          setProbeDistributionFactor(*DI,
+                                     1.0 * Candidate.CallsiteCount / SumOrigin);
         }
         return Inlined;
       }
     }
   } else {
     LLVM_DEBUG(dbgs() << "\nFailed to promote indirect call to "
                       << Candidate.CalleeSamples->getFuncName() << " because "
                       << Reason << "\n");
while (!CQueue.empty() && F.getInstructionCount() < SizeLimit) {
     CallBase *I = Candidate.CallInstr;
     Function *CalledFunction = I->getCalledFunction();
 
     if (CalledFunction == &F)
       continue;
     if (I->isIndirectCall()) {
       uint64_t Sum;
       auto CalleeSamples = findIndirectCallFunctionSamples(*I, Sum);
       uint64_t SumOrigin = Sum;
       Sum *= Candidate.CallsiteDistribution;
       for (const auto *FS : CalleeSamples) {
         // TODO: Consider disable pre-lTO ICP for MonoLTO as well
         if (LTOPhase == ThinOrFullLTOPhase::ThinLTOPreLink) {
           findExternalInlineCandidate(FS, InlinedGUIDs, SymbolMap,
                                       PSI->getOrCompHotCountThreshold());
           continue;
         }