Update entry count for cold calls

[david2050]

Summary

Profile sample files include the number of times each entry or inlined
call site is sampled. This is translated into the entry count metadta
on functions.

When sample data is being read, if a call site that was inlined
in the sample program is considered cold and not inlined, then
the entry count of the out-of-line functions does not reflect
the current compilation.

In this patch, we note call sites where the function was not inlined
and as a last action of the sample profile loading, we update the
called function's entry count to reflect the calls from these
call sites which are not included in the profile file.

[david2050]

remove facebook marker

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

770

Probably name it differently. They are calls that are inlined in profiled binary but not in optimized binary. 'Coldness' might be just one of the reasons. At least make the comment clearer.

test/Transforms/SampleProfile/inline-cold.ll

24

Add checking of entry profile meta data here.

[david2050]

address review comments

[davidxl]

test/Transforms/SampleProfile/inline-cold.ll

25

it is better to match the expected profile count here using name reference in filecheck.

[david2050]

refine test

[david2050]

thanks for the suggestions

test/Transforms/SampleProfile/inline-cold.ll

25

Thanks for the suggestion.

[david2050]

ping

[wmi]

Sorry for the late reply. I didn't notice the thread.

I admit the problem that the patch describes introduces some impreciseness to the profile annotated, but a problem is that the patch only updates the entry counts of outlined functions, but the profiles inside of those outlined functions are not updated. Ideally if the entry count of a outline function is doubled, maybe the callsite count inside of the function also should be doubled too?

I am not saying it is necessary to keep all the profiles consistent, after all it is about cold place. But I like to know whether the patch is trying to solve a theoretical problem or motivated by a real case.

Thanks,
Wei.

lib/Transforms/IPO/SampleProfile.cpp

866–867

Can be merged into one line.

1603–1609

Maybe merged to:
ECount = Callee->getEntryCount()
Callee->setEntryCount(pair.getSecond() + ECount ? ECount.getCount() : 0);

test/Transforms/SampleProfile/Inputs/inline-cold.prof

2–10

Better to make the function total count equal to the sum of the counts inside of the function because such inconsistency can cause trouble sometimes.

test/Transforms/SampleProfile/inline-cold.ll

31–32

%0, %1... should be renamed in testcase.

[david2050]

Thanks for the review.
This was intended to balance the function updateCalleeCount in InlineFunction.cpp which simply decrements the entry count.

[wmi]

This was intended to balance the function updateCalleeCount in InlineFunction.cpp which simply decrements the entry count.

Thanks. After inlining a function, updateCalleeCount and updateCallProfile are used to update entry count of callee and meta data for call instructions. Here what the patch is doing looks like to update the profile when outlining an inline instance, which is the opposite of inlining, so should we update the meta data for call instructions inside the callee as well?

[david2050]

refactor and update

[david2050]

Sorry for the delay.
Refactored the code in InlineFunction for reuse and to update weights uniformly.

[wmi]

lib/IR/Function.cpp

1403–1404 ↗

(On Diff #180834)

It seems to me a little bit weird to have some structure specific for inliner in the interface of llvm IR. It looks more like a transformation utility and better to stay under lib/Transforms/Utils/. We may create a new file for it if there is nowhere else to put.

And maybe make the name more explicit like updateCalleeProfile.

lib/Transforms/IPO/SampleProfile.cpp

310

I don't find where samples is used?

831

Indirect call is skipped in the current patch, do you think it is better to handle it as well? I don't have strong opionion on this since it is cold.

1470

inlineResult.second is never used. What is the intention to change the interface of inlineHotFunctions?

[david2050]

lib/IR/Function.cpp

1403–1404 ↗

(On Diff #180834)

I will try putting it back into InlineFunction and exporting it as a static method

lib/Transforms/IPO/SampleProfile.cpp

310

part of a subsequent patch, I will remove from here.

[david2050]

remove some unneeded changes, refactor location of updateProfileData, now updateProfileCallee

[wmi]

lib/Transforms/Utils/InlineFunction.cpp

1486–1494 ↗

(On Diff #181656)

Maybe use a lambda here to remove the code duplication?

test/Transforms/SampleProfile/entry_counts_cold.ll

67–68 ↗

(On Diff #181656)

Add check for the updated profile of the call instruction inside of bar?

[david2050]

refactor common code, update test

[david2050]

lib/Transforms/Utils/InlineFunction.cpp

1486–1494 ↗

(On Diff #181656)

I refactored to pull the common out of the iff and trust unswitching :-)

[wmi]

include/llvm/DebugInfo/DIContext.h

101–105 ↗

(On Diff #181685)

Are the changes in this file belonging to some other patch?

lib/Transforms/IPO/SampleProfile.cpp

306–307

Can we make the comment more precise? I think NotInlinedProfileInfo is an accumulated information from all the callsites to the same function not being inlined but with inline instance in the profile. It is not a local information for each callsite.

767

Can we change the name to be more self-explain or give it a comment? These are not just calls not being inlined, but also with inline instance in the profile.

test/Transforms/SampleProfile/entry_counts_cold.ll

2 ↗

(On Diff #181685)

Please briefly describe what the test is up for so people don't have to search the commit history to find out.

26 ↗

(On Diff #181685)

bar is not inlined.

[david2050]

address comments

[wmi]

Sorry it takes me some time to test the change on our major benchmarks. The results are neutral.

LGTM.

[wenlei]

@wmi @davidxl FYI, we have an internal patch to address this issue further. This patch only scales up entry count of outline function for cold callsites, thus losing context-sensitiveness. But ideally, we could reuse the entire profile from that inline context, and merge it all back to the outlined function. We changed the function annotation/inlining order to be top-down, then as we decide to not inline a cold call site, we merge the entire FunctionSamples of that inlinee back to outline counterpart, and since we process functions in top-down order (ignoring recursion for a second), annotation of the outline callee is able to use post merge FunctionSamples which is more accurate than simple scaling of entry count. (Doing context-sensitive inlining top-down has other benefits too, as it allows specialization whiling inlining which helps maximize the benefit of having context sensitive profile)

We'll upstream the changes later if you think it will be useful.

[davidxl]

Wenlei, this sounds like a good idea. Patches are welcome!

[wenlei]

In D52845#1707286, @davidxl wrote:

Wenlei, this sounds like a good idea. Patches are welcome!

Sorry for the delay. I just sent D70584, D70653 and D70655. There's another one that reduces the oscillation between sample loader inlining and CGSCC inlining for small inlinees to better preserve CS profile, and I'll also upstream that later. We got small wins with these changes.

lib/Transforms/IPO/SampleProfile.cpp

protected:
   /// Total number of samples collected in this profile.
   ///
   /// This is the sum of all the samples collected in all the functions executed
   /// at runtime.
   uint64_t TotalCollectedSamples = 0;
 
   /// Optimization Remark Emitter used to emit diagnostic remarks.
   OptimizationRemarkEmitter *ORE = nullptr;
+
+  // Record entry counts for call sites which were inlined in the sampled
+  // program but are not inlined (perhaps because they are cold)
+  DenseMap<Function *, uint64_t> notInlinedCounts;
 };
 
 class SampleProfileLoaderLegacyPass : public ModulePass {
 public:
   // Class identification, replacement for typeinfo
   static char ID;
 
   SampleProfileLoaderLegacyPass(StringRef Name = SampleProfileFile,
                                 bool IsThinLTOPreLink = false)
       : ModulePass(ID), SampleLoader(Name, IsThinLTOPreLink,

 /// \param F function to perform iterative inlining.
 /// \param InlinedGUIDs a set to be updated to include all GUIDs that are
 ///     inlined in the profiled binary.
 ///
 /// \returns True if there is any inline happened.
 bool SampleProfileLoader::inlineHotFunctions(
     Function &F, DenseSet<GlobalValue::GUID> &InlinedGUIDs) {
   DenseSet<Instruction *> PromotedInsns;
+
+  // Map call sites to their sample information. Call sites which
+  // are "hot" and inlined will be replaced leaving only the cold
+  // ones.
+  DenseMap<Instruction *, const FunctionSamples *> notInlinedCallSites;
+
   bool Changed = false;
   while (true) {
     bool LocalChanged = false;
     SmallVector<Instruction *, 10> CIS;
     for (auto &BB : F) {
       bool Hot = false;
       SmallVector<Instruction *, 10> Candidates;
       for (auto &I : BB.getInstList()) {
         const FunctionSamples *FS = nullptr;
         if ((isa<CallInst>(I) || isa<InvokeInst>(I)) &&
             !isa<IntrinsicInst>(I) && (FS = findCalleeFunctionSamples(I))) {
           Candidates.push_back(&I);
+          if (FS->getEntrySamples() > 0)
+            notInlinedCallSites.try_emplace(&I, FS);
           if (callsiteIsHot(FS, PSI))
             Hot = true;
         }
       }
       if (Hot) {
         CIS.insert(CIS.begin(), Candidates.begin(), Candidates.end());
       }
     }
for (auto I : CIS) {
               isLegalToPromote(CallSite(I), R->getValue(), &Reason)) {
             uint64_t C = FS->getEntrySamples();
             Instruction *DI =
                 pgo::promoteIndirectCall(I, R->getValue(), C, Sum, false, ORE);
             Sum -= C;
             PromotedInsns.insert(I);
             // If profile mismatches, we should not attempt to inline DI.
             if ((isa<CallInst>(DI) || isa<InvokeInst>(DI)) &&
                 inlineCallInstruction(DI))
               LocalChanged = true;
           } else {
             LLVM_DEBUG(dbgs()
                        << "\nFailed to promote indirect call to "
                        << CalleeFunctionName << " because " << Reason << "\n");
           }
         }
       } else if (CalledFunction && CalledFunction->getSubprogram() &&
                  !CalledFunction->isDeclaration()) {
-        if (inlineCallInstruction(I))
+        if (inlineCallInstruction(I)) {
+          notInlinedCallSites.erase(I);
           LocalChanged = true;
+        }
       } else if (IsThinLTOPreLink) {
         findCalleeFunctionSamples(*I)->findInlinedFunctions(
             InlinedGUIDs, F.getParent(), PSI->getOrCompHotCountThreshold());
       }
     }
     if (LocalChanged) {
       Changed = true;
     } else {
       break;
     }
   }
+
+  // For callsites with profile data from sample file but which were not
+  // inlined, we save the entry samples for later addition to the
+  // called functions.
+  for (const auto &Pair : notInlinedCallSites) {
+    Instruction *I = Pair.getFirst();
+    Function *Callee = CallSite(I).getCalledFunction();
+    if (!Callee || Callee->isDeclaration())
+      continue;
+    auto pair = notInlinedCounts.try_emplace(Callee, 0);
+    const auto *FS = Pair.getSecond();
+    pair.first->second += FS->getEntrySamples();
+  }
   return Changed;
 }
 
 /// Find equivalence classes for the given block.
 ///
 /// This finds all the blocks that are guaranteed to execute the same
 /// number of times as \p BB1. To do this, it traverses all the
 /// descendants of \p BB1 in the dominator or post-dominator tree.
bool SampleProfileLoader::emitAnnotations(Function &F) {
 
   if (getFunctionLoc(F) == 0)
     return false;
 
   LLVM_DEBUG(dbgs() << "Line number for the first instruction in "
                     << F.getName() << ": " << getFunctionLoc(F) << "\n");
 
   DenseSet<GlobalValue::GUID> InlinedGUIDs;
   Changed |= inlineHotFunctions(F, InlinedGUIDs);
 
   // Compute basic block weights.
   Changed |= computeBlockWeights(F);
 
   if (Changed) {
     // Add an entry count to the function using the samples gathered at the
     // function entry.
     // Sets the GUIDs that are inlined in the profiled binary. This is used
bool SampleProfileLoader::runOnModule(Module &M, ModuleAnalysisManager *AM,
   }
 
   bool retval = false;
   for (auto &F : M)
     if (!F.isDeclaration()) {
       clearFunctionData();
       retval |= runOnFunction(F, AM);
     }
+
+  // Account for cold calls not inlined by adding the entry
+  // counts from the inline call sites in the profile to the
+  // entry count for the call.
+  for (const auto &pair : notInlinedCounts) {
+    Function *Callee = pair.getFirst();
+    assert(!Callee->isDeclaration());
+    uint64_t CallCount = pair.getSecond();
+    uint64_t CalleeCount = 0;
+    if (auto ECount = Callee->getEntryCount())
+      CalleeCount = ECount.getCount();
+
+    CalleeCount += CallCount;
+    Callee->setEntryCount(CalleeCount);
+  }
+
   return retval;
 }
 
 bool SampleProfileLoaderLegacyPass::runOnModule(Module &M) {
   ACT = &getAnalysis<AssumptionCacheTracker>();
   TTIWP = &getAnalysis<TargetTransformInfoWrapperPass>();
   ProfileSummaryInfo *PSI =
       getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();

test/Transforms/SampleProfile/Inputs/inline-cold.prof

+main:225715:10
+ 2.1: 5553
+ 3: 5391
+ 3.1: _Z3sumii:586
+  0: 5279
+  1: 5279
+  2: 5279
+_Z3sumii:400:20
+ 1: 20

test/Transforms/SampleProfile/inline-cold.ll

+; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/inline-cold.prof -S | FileCheck %s
+; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/inline-cold.prof -S | FileCheck %s
+
+; Original C++ test case
+;
+; #include <stdio.h>
+;
+; int sum(int x, int y) {
+;   return x + y;
+; }
+;
+; int main() {
+;   int s, i = 0;
+;   while (i++ < 20000 * 20000)
+;     if (i != 100) s = sum(i, s); else s = 30;
+;   printf("sum is %d\n", s);
+;   return 0;
+; }
+;
+@.str = private unnamed_addr constant [11 x i8] c"sum is %d\0A\00", align 1
+
+; Function Attrs: nounwind uwtable
+define i32 @_Z3sumii(i32 %x, i32 %y) !dbg !4 {
+entry:
+; CHECK: !prof
+  %x.addr = alloca i32, align 4
+  %y.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  store i32 %y, i32* %y.addr, align 4
+  %0 = load i32, i32* %x.addr, align 4, !dbg !11
+  %1 = load i32, i32* %y.addr, align 4, !dbg !11
+  %add = add nsw i32 %0, %1, !dbg !11
+  ret i32 %add, !dbg !11
+}
+
+; Function Attrs: uwtable
+define i32 @main() !dbg !7 {
+entry:
+; CHECK: !prof
+  %retval = alloca i32, align 4
+  %s = alloca i32, align 4
+  %i = alloca i32, align 4
+  store i32 0, i32* %retval
+  store i32 0, i32* %i, align 4, !dbg !12
+  br label %while.cond, !dbg !13
+
+while.cond:                                       ; preds = %if.end, %entry
+  %0 = load i32, i32* %i, align 4, !dbg !14
+  %inc = add nsw i32 %0, 1, !dbg !14
+  store i32 %inc, i32* %i, align 4, !dbg !14
+  %cmp = icmp slt i32 %0, 400000000, !dbg !14
+  br i1 %cmp, label %while.body, label %while.end, !dbg !14
+
+while.body:                                       ; preds = %while.cond
+  %1 = load i32, i32* %i, align 4, !dbg !16
+  %cmp1 = icmp ne i32 %1, 100, !dbg !16
+  br i1 %cmp1, label %if.then, label %if.else, !dbg !16
+
+
+if.then:                                          ; preds = %while.body
+  %2 = load i32, i32* %i, align 4, !dbg !18
+  %3 = load i32, i32* %s, align 4, !dbg !18
+  %call = call i32 @_Z3sumii(i32 %2, i32 %3), !dbg !18
+; CHECK: call i32 @_Z3sumii
+  store i32 %call, i32* %s, align 4, !dbg !18
+  br label %if.end, !dbg !18
+
+if.else:                                          ; preds = %while.body
+  store i32 30, i32* %s, align 4, !dbg !20
+  br label %if.end
+
+if.end:                                           ; preds = %if.else, %if.then
+  br label %while.cond, !dbg !22
+
+while.end:                                        ; preds = %while.cond
+  %4 = load i32, i32* %s, align 4, !dbg !24
+  %call2 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([11 x i8], [11 x i8]* @.str, i32 0, i32 0), i32 %4), !dbg !24
+  ret i32 0, !dbg !25
+}
+
+declare i32 @printf(i8*, ...) #2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!8, !9}
+!llvm.ident = !{!10}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, producer: "clang version 3.5 ", isOptimized: false, emissionKind: NoDebug, file: !1, enums: !2, retainedTypes: !2, globals: !2, imports: !2)
+!1 = !DIFile(filename: "calls.cc", directory: ".")
+!2 = !{}
+!4 = distinct !DISubprogram(name: "sum", line: 3, isLocal: false, isDefinition: true, virtualIndex: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !0, scopeLine: 3, file: !1, scope: !5, type: !6, retainedNodes: !2)
+!5 = !DIFile(filename: "calls.cc", directory: ".")
+!6 = !DISubroutineType(types: !2)
+!7 = distinct !DISubprogram(name: "main", line: 7, isLocal: false, isDefinition: true, virtualIndex: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !0, scopeLine: 7, file: !1, scope: !5, type: !6, retainedNodes: !2)
+!8 = !{i32 2, !"Dwarf Version", i32 4}
+!9 = !{i32 1, !"Debug Info Version", i32 3}
+!10 = !{!"clang version 3.5 "}
+!11 = !DILocation(line: 4, scope: !4)
+!12 = !DILocation(line: 8, scope: !7)
+!13 = !DILocation(line: 9, scope: !7)
+!14 = !DILocation(line: 9, scope: !15)
+!15 = !DILexicalBlockFile(discriminator: 2, file: !1, scope: !7)
+!16 = !DILocation(line: 10, scope: !17)
+!17 = distinct !DILexicalBlock(line: 10, column: 0, file: !1, scope: !7)
+!18 = !DILocation(line: 10, scope: !19)
+!19 = !DILexicalBlockFile(discriminator: 2, file: !1, scope: !17)
+!20 = !DILocation(line: 10, scope: !21)
+!21 = !DILexicalBlockFile(discriminator: 4, file: !1, scope: !17)
+!22 = !DILocation(line: 10, scope: !23)
+!23 = !DILexicalBlockFile(discriminator: 6, file: !1, scope: !17)
+!24 = !DILocation(line: 11, scope: !7)
+!25 = !DILocation(line: 12, scope: !7)
+; CHECK: !{!"function_entry_count", i64 5300}
+; CHECK: !{!"function_entry_count", i64 11}