[SamplePGO] Skip inlinee profile scaling for sample loader inlining

[wenlei]

Summary

For CGSCC inline, we need to scale down a function's branch weights and entry counts when thee it's inlined at a callsite. This is done through updateCallProfile. Additionally, we also scale the weigths for the inlined clone based on call site count in updateCallerBFI. Neither is needed for inlining during sample profile loader as it's using context profile that is separated from inlinee's own profile. This change skip the inlinee profile scaling for sample loader inlining.

[wenlei]

We have this change internally, and it shows a 0.2-0.3% geomean win on spec2017 with CSSPGO (from small wins on larger ones like xalancbmk). I think it should help baseline AutoFDO too.

[wenlei]

@wmi any concern with landing this change? I don't expect any issues, but just to be prudent do you want to experiment before this is landed?

[wmi]

In D98187#2618605, @wenlei wrote:

@wmi any concern with landing this change? I don't expect any issues, but just to be prudent do you want to experiment before this is landed?

That is a nice catch! I will definitely experiment it and expect some improvement from it. Will have the data tomorrow and get back.

[wmi]

In D98187#2618612, @wmi wrote:

In D98187#2618605, @wenlei wrote:

@wmi any concern with landing this change? I don't expect any issues, but just to be prudent do you want to experiment before this is landed?

That is a nice catch! I will definitely experiment it and expect some improvement from it. Will have the data tomorrow and get back.

I got ~0.2% improvement on our search benchmark. That is a nice improvement. Thanks!

[wenlei]

In D98187#2619936, @wmi wrote:

In D98187#2618612, @wmi wrote:

In D98187#2618605, @wenlei wrote:

@wmi any concern with landing this change? I don't expect any issues, but just to be prudent do you want to experiment before this is landed?

That is a nice catch! I will definitely experiment it and expect some improvement from it. Will have the data tomorrow and get back.

I got ~0.2% improvement on our search benchmark. That is a nice improvement. Thanks!

Thanks for the measurement, great to know it helps.

llvm/include/llvm/Transforms/Utils/Cloning.h

 /// the auxiliary results produced by it.
 class InlineFunctionInfo {
 public:
   explicit InlineFunctionInfo(
       CallGraph *cg = nullptr,
       function_ref<AssumptionCache &(Function &)> GetAssumptionCache = nullptr,
       ProfileSummaryInfo *PSI = nullptr,
       BlockFrequencyInfo *CallerBFI = nullptr,
-      BlockFrequencyInfo *CalleeBFI = nullptr)
+      BlockFrequencyInfo *CalleeBFI = nullptr, bool UpdateProfile = true)
       : CG(cg), GetAssumptionCache(GetAssumptionCache), PSI(PSI),
-        CallerBFI(CallerBFI), CalleeBFI(CalleeBFI) {}
+        CallerBFI(CallerBFI), CalleeBFI(CalleeBFI),
+        UpdateProfile(UpdateProfile) {}
 
   /// If non-null, InlineFunction will update the callgraph to reflect the
   /// changes it makes.
   CallGraph *CG;
   function_ref<AssumptionCache &(Function &)> GetAssumptionCache;
   ProfileSummaryInfo *PSI;
   BlockFrequencyInfo *CallerBFI, *CalleeBFI;
 
   /// InlineFunction fills this in with all static allocas that get copied into
   /// the caller.
   SmallVector<AllocaInst *, 4> StaticAllocas;
 
   /// InlineFunction fills this in with callsites that were inlined from the
   /// callee. This is only filled in if CG is non-null.
   SmallVector<WeakTrackingVH, 8> InlinedCalls;
 
   /// All of the new call sites inlined into the caller.
   ///
   /// 'InlineFunction' fills this in by scanning the inlined instructions, and
   /// only if CG is null. If CG is non-null, instead the value handle
   /// `InlinedCalls` above is used.
   SmallVector<CallBase *, 8> InlinedCallSites;
 
+  /// Update profile for callee as well as cloned version. We need to do this
+  /// for regular inlining, but not for inlining from sample profile loader.
+  bool UpdateProfile;
+
   void reset() {
     StaticAllocas.clear();
     InlinedCalls.clear();
     InlinedCallSites.clear();
   }
 };
 
 /// This function inlines the called function into the basic

llvm/lib/Transforms/IPO/SampleProfile.cpp

ORE->emit(OptimizationRemarkAnalysis(CSINLINE_DEBUG, "InlineFail", DLoc, BB)
               << "incompatible inlining");
     return false;
   }
 
   if (!Cost)
     return false;
 
   InlineFunctionInfo IFI(nullptr, GetAC);
+  IFI.UpdateProfile = false;
   if (InlineFunction(CB, IFI).isSuccess()) {
     // The call to InlineFunction erases I, so we can't pass it here.
     emitInlinedInto(*ORE, DLoc, BB, *CalledFunction, *BB->getParent(), Cost,
                     true, CSINLINE_DEBUG);
 
     // Now populate the list of newly exposed call sites.
     if (InlinedCallSites) {
       InlinedCallSites->clear();

llvm/lib/Transforms/Utils/InlineFunction.cpp

CloneAndPruneFunctionInto(Caller, CalledFunc, VMap,
                               &InlinedFunctionInfo, &CB);
     // Remember the first block that is newly cloned over.
     FirstNewBlock = LastBlock; ++FirstNewBlock;
 
     // Insert retainRV/clainRV runtime calls.
     if (objcarc::hasAttachedCallOpBundle(&CB))
       inlineRetainOrClaimRVCalls(CB, Returns);
 
+    // Updated caller/callee profiles only when requested. For sample loader
+    // inlining, the context-sensitive inlinee profile doesn't need to be
+    // subtracted from callee profile, and the inlined clone also doesn't need
+    // to be scaled based on call site count.
+    if (IFI.UpdateProfile) {
       if (IFI.CallerBFI != nullptr && IFI.CalleeBFI != nullptr)
         // Update the BFI of blocks cloned into the caller.
         updateCallerBFI(OrigBB, VMap, IFI.CallerBFI, IFI.CalleeBFI,
                         CalledFunc->front());
 
       updateCallProfile(CalledFunc, VMap, CalledFunc->getEntryCount(), CB,
                         IFI.PSI, IFI.CallerBFI);
+    }
 
     // Inject byval arguments initialization.
     for (std::pair<Value*, Value*> &Init : ByValInit)
       HandleByValArgumentInit(Init.first, Init.second, Caller->getParent(),
                               &*FirstNewBlock, IFI);
 
     Optional<OperandBundleUse> ParentDeopt =
         CB.getOperandBundle(LLVMContext::OB_deopt);