Keep import function list for inlinee profile update

[wenlei]

Summary

When adjusting function entry counts after inlining, Funciton::setEntryCount is called without providing an import function list. The side effect of that is the previously set import function list will be dropped. The import function list is used by ThinLTO to help import hot cross module callee for LTO inlining, so dropping that during ThinLTO pre-link may adversely affect LTO inlining. The fix is to keep the list while updating entry counts for inlining.

Separate but related, we don't need to update/scale profile for callee or cloned inlinee if we decided to inline a callee from sample profile loader with context-sensitive profile. This is fixed by passing a flag through InlineFunctionInfo to tell inliner when not to update profile.

[wenlei]

This is a minimal point fix (was trying to keep internal patch minimum), but it might actually make sense to have setEntryCount always copy over the existing import list (if it exists), instead of adding updateEntryCount? Or even further, we could have a separate metadata for the import list, instead of piggyback on function_entry_count? To me, it seems a bit weird to embed import list as part of function_entry_count - wondering what's the reason behind that?

[wmi]

Thanks for the patch!

This is a minimal point fix (was trying to keep internal patch minimum), but it might actually make sense to have setEntryCount always copy over the existing import list (if it exists), instead of adding updateEntryCount? Or even further, we could have a separate metadata for the import list, instead of piggyback on function_entry_count? To me, it seems a bit weird to embed import list as part of function_entry_count - wondering what's the reason behind that?

I agree setEntryCount always copying the existing import list is better than adding updateEntryCount. Adding a separate metadata also sounds good to me. I don't know the history why enbedding import list in function_entry_count. @tejohnson may know.

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

208–211 ↗

(On Diff #227522)

I don't see where UpdateProfile is used.

llvm/test/Transforms/SampleProfile/inline-callee-update.ll

78–81

I am curious why these two functions have non empty import lists because all the functions are defined within the module.

Then I find indirect call targets with inline instance are all added to import list. That is unnecessary if the call targets are defined in current module. That could be improved. At least, the test shouldn't depend on it.

[wenlei]

I agree setEntryCount always copying the existing import list is better than adding updateEntryCount. Adding a separate metadata also sounds good to me. I don't know the history why enbedding import list in function_entry_count. @tejohnson may know.

Updated the diff to have setEntryCount always copying the existing import list for now. I'd be happy to add a new metadata for import list too.. Thanks for reviewing!

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

208–211 ↗

(On Diff #227522)

oops, merge error, thanks for catching this. fixed.

llvm/test/Transforms/SampleProfile/inline-callee-update.ll

78–81

Makes sense, changed the test case to rely only on cross module (declare-only) import. (we can trim same module indirect callee import in a separate diff)

[wenlei]

address review feedback

[wenlei]

update the right version of setEntryCount

[tejohnson]

Nice catch on the dropped import lists!

llvm/lib/Transforms/IPO/SampleProfile.cpp

876 ↗

(On Diff #227728)

Document constant parameters.

llvm/lib/Transforms/Utils/InlineFunction.cpp

1742

Can you split this into a separate patch (with a test) since it isn't related to the headline of this patch? Are we currently getting incorrect profiles without this fix?

Suggest pulling the if (IFI.UpdateProfile) checks out into a single if for clarity.

llvm/test/Transforms/SampleProfile/inline-callee-update.ll

2

Is this intended to be here?

[wmi]

Could you add a testcase to verify that no profile update for inlining in sampleloader pass?

llvm/test/Transforms/SampleProfile/inline-callee-update.ll

7

Please use "opt -instnamer" to change %1, %2, ... to names more than just numbers.

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

876 ↗

(On Diff #227728)

Reverted this as part of the split.

llvm/lib/Transforms/Utils/InlineFunction.cpp

1742

Sure.. the profile we are getting is still incorrect for some cases because line 1517 is still executed for inlining with CS profile. But I will put that into a separate diff later with its own test case.

llvm/test/Transforms/SampleProfile/inline-callee-update.ll

2

oops.. removed internal marker, but left the comment there.

7

Done.

[wenlei]

Split UpdateProfile part, address other feedbacks.

[tejohnson]

Thanks for splitting the patch. This one lgtm, but please wait to see if @wmi has any more comments. Also the summary needs an update (to remove the bit about the CS profile fix).

[wmi]

I don't have further comment. It LGTM. I am doing some performance test for it and will report back its performance impact on our side (expect to be positive).

[wmi]

I saw 0.3% improvement in our server benchmark. That is a great improvement!

[wenlei]

In D69736#1734892, @wmi wrote:

I saw 0.3% improvement in our server benchmark. That is a great improvement!

Thanks for sharing the data! Glad that it helped.

llvm/lib/IR/Function.cpp

 
 void Function::setEntryCount(ProfileCount Count,
                              const DenseSet<GlobalValue::GUID> *S) {
   assert(Count.hasValue());
 #if !defined(NDEBUG)
   auto PrevCount = getEntryCount();
   assert(!PrevCount.hasValue() || PrevCount.getType() == Count.getType());
 #endif
+
+  auto ImportGUIDs = getImportGUIDs();
+  if (S == nullptr && ImportGUIDs.size())
+    S = &ImportGUIDs;
+
   MDBuilder MDB(getContext());
   setMetadata(
       LLVMContext::MD_prof,
       MDB.createFunctionEntryCount(Count.getCount(), Count.isSynthetic(), S));
 }
 
 void Function::setEntryCount(uint64_t Count, Function::ProfileCountType Type,
                              const DenseSet<GlobalValue::GUID> *Imports) {

llvm/lib/Transforms/Utils/InlineFunction.cpp

void llvm::updateProfileCallee(
 
   // Since CallSiteCount is an estimate, it could exceed the original callee
   // count and has to be set to 0 so guard against underflow.
   if (entryDelta < 0 && static_cast<uint64_t>(-entryDelta) > priorEntryCount)
     newEntryCount = 0;
   else
     newEntryCount = priorEntryCount + entryDelta;
 
-  Callee->setEntryCount(newEntryCount);
-
   // During inlining ?
   if (VMap) {
     uint64_t cloneEntryCount = priorEntryCount - newEntryCount;
     for (auto const &Entry : *VMap)
       if (isa<CallInst>(Entry.first))
         if (auto *CI = dyn_cast_or_null<CallInst>(Entry.second))
           CI->updateProfWeight(cloneEntryCount, priorEntryCount);
   }
+
+  if (entryDelta) {
+    Callee->setEntryCount(newEntryCount);
+
     for (BasicBlock &BB : *Callee)
       // No need to update the callsite if it is pruned during inlining.
       if (!VMap || VMap->count(&BB))
         for (Instruction &I : BB)
           if (CallInst *CI = dyn_cast<CallInst>(&I))
             CI->updateProfWeight(newEntryCount, priorEntryCount);
   }
+}
 
 /// This function inlines the called function into the basic block of the
 /// caller. This returns false if it is not possible to inline this call.
 /// The program is still in a well defined state if this occurs though.
 ///
 /// Note that this only does one level of inlining.  For example, if the
 /// instruction 'call B' is inlined, and 'B' calls 'C', then the call to 'C' now
 /// exists in the instruction stream.  Similarly this will inline a recursive
Function::iterator FirstNewBlock;
     // happy with whatever the cloner can do.
     CloneAndPruneFunctionInto(Caller, CalledFunc, VMap,
                               /*ModuleLevelChanges=*/false, Returns, ".i",
                               &InlinedFunctionInfo, TheCall);
     // Remember the first block that is newly cloned over.
     FirstNewBlock = LastBlock; ++FirstNewBlock;
 
     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(), TheCall,
                       IFI.PSI, IFI.CallerBFI);
 
     // Inject byval arguments initialization.
     for (std::pair<Value*, Value*> &Init : ByValInit)

llvm/test/Transforms/SampleProfile/Inputs/inline-callee-update.prof

+test_sample_loader_inline:63067:0
+ 1: sample_loader_inlinee:40000
+sample_loader_inlinee:3000:0
+ 1: direct_leaf_func:35000
+  11: 3000
+test_cgscc_inline:63067:0
+ 1: sample_loader_inlinee:1
+cgscc_inlinee:3000:0
+ 1: direct_leaf_func:35000
+  11: 3000
 No newline at end of file

llvm/test/Transforms/SampleProfile/inline-callee-update.ll

+; Make sure Import GUID list for ThinLTO properly maintained while update function's entry count for inlining
+
+; RUN: opt < %s -passes='thinlto-pre-link<O2>' -pgo-kind=pgo-sample-use-pipeline -sample-profile-file=%S/Inputs/inline-callee-update.prof -S | FileCheck %s
+
+@y = global i32* ()* null, align 8
+@z = global i32* ()* null, align 8
+
+define i32* @sample_loader_inlinee() !dbg !3 {
+bb:
+  %tmp = call i32* @direct_leaf_func(i32* null), !dbg !4
+  %cmp = icmp ne i32* %tmp, null
+  br i1 %cmp, label %then, label %else
+
+then:                                             ; preds = %bb
+  %tmp1 = load i32* ()*, i32* ()** @z, align 8, !dbg !5
+  %tmp2 = call i32* %tmp1(), !dbg !5
+  ret i32* %tmp2
+
+else:                                             ; preds = %bb
+  ret i32* null
+}
+
+define i32* @cgscc_inlinee() !dbg !6 {
+bb:
+  %tmp = call i32* @direct_leaf_func(i32* null), !dbg !7
+  %cmp = icmp ne i32* %tmp, null
+  br i1 %cmp, label %then, label %else
+
+then:                                             ; preds = %bb
+  %tmp1 = load i32* ()*, i32* ()** @y, align 8, !dbg !8
+  %tmp2 = call i32* %tmp1(), !dbg !8
+  ret i32* %tmp2
+
+else:                                             ; preds = %bb
+  ret i32* null
+}
+
+define i32* @test_sample_loader_inline(void ()* %arg) !dbg !9 {
+bb:
+  %tmp = call i32* @sample_loader_inlinee(), !dbg !10
+  ret i32* %tmp
+}
+
+define i32* @test_cgscc_inline(void ()* %arg) !dbg !11 {
+bb:
+  %tmp = call i32* @cgscc_inlinee(), !dbg !12
+  ret i32* %tmp
+}
+
+declare i32* @direct_leaf_func(i32*)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug)
+!1 = !DIFile(filename: "test.cc", directory: "/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = distinct !DISubprogram(name: "sample_loader_inlinee", scope: !1, file: !1, line: 11, spFlags: DISPFlagDefinition, unit: !0)
+!4 = !DILocation(line: 12, scope: !3)
+!5 = !DILocation(line: 13, scope: !3)
+!6 = distinct !DISubprogram(name: "cgscc_inlinee", scope: !1, file: !1, line: 31, spFlags: DISPFlagDefinition, unit: !0)
+!7 = !DILocation(line: 32, scope: !6)
+!8 = !DILocation(line: 33, scope: !6)
+!9 = distinct !DISubprogram(name: "test_sample_loader_inline", scope: !1, file: !1, line: 3, spFlags: DISPFlagDefinition, unit: !0)
+!10 = !DILocation(line: 4, scope: !9)
+!11 = distinct !DISubprogram(name: "test_cgscc_inline", scope: !1, file: !1, line: 20, spFlags: DISPFlagDefinition, unit: !0)
+!12 = !DILocation(line: 21, scope: !11)
+
+; Make sure the ImportGUID stays with entry count metadata for ThinLTO-PreLink
+; CHECK: distinct !DISubprogram(name: "sample_loader_inlinee"
+; CHECK-NEXT: {!"function_entry_count", i64 1, i64 -9171813444624716006}
+; CHECK: distinct !DISubprogram(name: "cgscc_inlinee"
+; CHECK-NEXT: !{!"function_entry_count", i64 0, i64 -9171813444624716006}