Do not want to use BFI to get profile count for sample pgo

[danielcdh]

Summary

For SamplePGO, we already record the callsite count in the call instruction itself. So we do not want to use BFI to get profile count as it is less accurate.

[eraman]

If the profile count based on BFI is completely reliable that you are discarding it, then shouldn't BFI->getProfileCount also return 0 for sampled profiling? I think this can end up in surprising behavior.

[danielcdh]

In D36025#824892, @eraman wrote:

If the profile count based on BFI is completely reliable that you are discarding it, then shouldn't BFI->getProfileCount also return 0 for sampled profiling? I think this can end up in surprising behavior.

We only want to do this on callsites, as we still rely on BFI to get other block frequencies.

[eraman]

In D36025#824907, @danielcdh wrote:

In D36025#824892, @eraman wrote:

If the profile count based on BFI is completely reliable that you are discarding it, then shouldn't BFI->getProfileCount also return 0 for sampled profiling? I think this can end up in surprising behavior.

We only want to do this on callsites, as we still rely on BFI to get other block frequencies.

So, either the BFI based count is usable in sample PGO or not. If it is the latter, I think it should not be used at all. If it is usable (but not very accurate), then there is no harm in using in call.

Do we use the profile count of a non-call instruction anywhere for sample PGO? If we don't, may be assert in BFI->getProfileCount when we use sample PGO? Or alternatively, annotate all call instructions so that you can assert that every call has a metadata (which could be 0)?

[danielcdh]

In D36025#824921, @eraman wrote:

In D36025#824907, @danielcdh wrote:

In D36025#824892, @eraman wrote:

If the profile count based on BFI is completely reliable that you are discarding it, then shouldn't BFI->getProfileCount also return 0 for sampled profiling? I think this can end up in surprising behavior.

We only want to do this on callsites, as we still rely on BFI to get other block frequencies.

So, either the BFI based count is usable in sample PGO or not. If it is the latter, I think it should not be used at all. If it is usable (but not very accurate), then there is no harm in using in call.

It's usable. But inaccurate.

Do we use the profile count of a non-call instruction anywhere for sample PGO? If we don't, may be assert in BFI->getProfileCount when we use sample PGO?

Yes, there is already an assertion in ProfileSummaryInfo::getProfileCount to ensure Inst is a call/invoke instruction :)

Or alternatively, annotate all call instructions so that you can assert that every call has a metadata (which could be 0)?

In sample PGO, if the call count is 0, we don't set prof metadata to save space, as most of the call instructions will have 0 count.

[danielcdh]

ping...

[eraman]

I still feel this behavior is confusing, but I'll let David and Teresa to weigh in. Do you have any cases in real code where a block containing a call with no metadata has a high profile count? Is this masking a bug in some other code (BFI or sample profile loader)?

[eraman]

LGTM based on offline discussion with Dehao on why this could happen. Please add comments to the effect that if any callsite has a non-zero count, it gets a metadata and conversely any callsite without metadata has no count.

[danielcdh]

add comment

[danielcdh]

updated the implementation to include a flag to control whether to return 0 or None if the profile is missing for the entire function. PTAL.

[danielcdh]

Refactor the code after talking to Easwaran (Thanks!): Move the logic to IsCallsiteCold.

[danielcdh]

update test.

lib/Analysis/ProfileSummaryInfo.cpp

ProfileSummaryInfo::getProfileCount(const Instruction *Inst,
                                     BlockFrequencyInfo *BFI) {
   if (!Inst)
     return None;
   assert((isa<CallInst>(Inst) || isa<InvokeInst>(Inst)) &&
          "We can only get profile count for call/invoke instruction.");
   if (hasSampleProfile()) {
     // In sample PGO mode, check if there is a profile metadata on the
     // instruction. If it is present, determine hotness solely based on that,
-    // since the sampled entry count may not be accurate.
+    // since the sampled entry count may not be accurate. If there is no
+    // annotated on the instruction, it means that it is a cold callsite.
     uint64_t TotalCount;
     if (Inst->extractProfTotalWeight(TotalCount))
       return TotalCount;
+    return 0;
   }
   if (BFI)
     return BFI->getBlockProfileCount(Inst->getParent());
   return None;
 }
 
 /// Returns true if the function's entry is hot. If it returns false, it
 /// either means it is not hot or it is unknown whether it is hot or not (for

test/Bitcode/Inputs/thinlto-function-summary-callgraph-sample-profile-summary.ll

+; ModuleID = 'thinlto-function-summary-callgraph-profile-summary2.ll'
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @hot1() #1 {
+  ret void
+}
+define void @hot2() #1 {
+  ret void
+}
+define void @hot3() #1 {
+  ret void
+}
+define void @cold1() #1 {
+  ret void
+}
+define void @cold2() #1 {
+  ret void
+}
+define void @cold3() #1 {
+  ret void
+}
+define void @none1() #1 {
+  ret void
+}
+define void @none2() #1 {
+  ret void
+}
+define void @none3() #1 {
+  ret void
+}

test/Bitcode/thinlto-function-summary-callgraph-sample-profile-summary.ll

 ; Test to check the callgraph in summary when there is PGO
 ; RUN: opt -module-summary %s -o %t.o
 ; RUN: llvm-bcanalyzer -dump %t.o | FileCheck %s
-; RUN: opt -module-summary %p/Inputs/thinlto-function-summary-callgraph-profile-summary.ll -o %t2.o
+; RUN: opt -module-summary %p/Inputs/thinlto-function-summary-callgraph-sample-profile-summary.ll -o %t2.o
 ; RUN: llvm-lto -thinlto -o %t3 %t.o %t2.o
 ; RUN: llvm-bcanalyzer -dump %t3.thinlto.bc | FileCheck %s --check-prefix=COMBINED
 
 
 ; CHECK: <SOURCE_FILENAME
 ; "hot_function"
 ; CHECK-NEXT: <FUNCTION op0=0 op1=12
 ; "hot1"
 ; CHECK-NEXT: <FUNCTION op0=12 op1=4
 ; "hot2"
 ; CHECK-NEXT: <FUNCTION op0=16 op1=4
 ; "hot3"
 ; CHECK-NEXT: <FUNCTION op0=20 op1=4
 ; "hot4"
-; CHECK-NEXT: <FUNCTION op0=24 op1=4
+; CHECK-NEXT: <FUNCTION op0=24 op1=5
 ; "cold"
-; CHECK-NEXT: <FUNCTION op0=28 op1=4
+; CHECK-NEXT: <FUNCTION op0=29 op1=5
 ; "none1"
-; CHECK-NEXT: <FUNCTION op0=32 op1=5
+; CHECK-NEXT: <FUNCTION op0=34 op1=5
 ; "none2"
-; CHECK-NEXT: <FUNCTION op0=37 op1=5
+; CHECK-NEXT: <FUNCTION op0=39 op1=5
 ; "none3"
-; CHECK-NEXT: <FUNCTION op0=42 op1=5
+; CHECK-NEXT: <FUNCTION op0=44 op1=5
+; CHECK-NEXT: <FUNCTION op0=49 op1=5
+
 ; CHECK-LABEL:       <GLOBALVAL_SUMMARY_BLOCK
 ; CHECK-NEXT:    <VERSION
-; CHECK-NEXT:    <VALUE_GUID op0=25 op1=123/>
-; op4=hot1 op6=cold op8=hot2 op10=hot4 op12=none1 op14=hot3 op16=none2 op18=none3 op20=123
-; CHECK-NEXT:    <PERMODULE_PROFILE {{.*}} op4=1 op5=3 op6=5 op7=1 op8=2 op9=3 op10=4 op11=3 op12=6 op13=2 op14=3 op15=3 op16=7 op17=2 op18=8 op19=2 op20=25 op21=4/>
+; CHECK-NEXT:    <VALUE_GUID op0=26 op1=123/>
+; op4=none1 op6=hot1 op8=cold1 op10=none2 op12=hot2 op14=cold2 op16=none3 op18=hot3 op20=cold3 op22=123
+; CHECK-NEXT:    <PERMODULE_PROFILE {{.*}} op4=7 op5=1 op6=1 op7=3 op8=4 op9=1 op10=8 op11=1 op12=2 op13=3 op14=5 op15=1 op16=9 op17=1 op18=3 op19=3 op20=6 op21=1 op22=26 op23=4/>
 ; CHECK-NEXT:  </GLOBALVAL_SUMMARY_BLOCK>
 
 ; CHECK: <STRTAB_BLOCK
-; CHECK-NEXT: blob data = 'hot_functionhot1hot2hot3hot4coldnone1none2none3{{.*}}'
+; CHECK-NEXT: blob data = 'hot_functionhot1hot2hot3cold1cold2cold3none1none2none3{{.*}}'
 
 ; COMBINED:       <GLOBALVAL_SUMMARY_BLOCK
 ; COMBINED-NEXT:    <VERSION
 ; COMBINED-NEXT:    <VALUE_GUID
 ; COMBINED-NEXT:    <VALUE_GUID
 ; COMBINED-NEXT:    <VALUE_GUID
 ; COMBINED-NEXT:    <VALUE_GUID
 ; COMBINED-NEXT:    <VALUE_GUID
 ; COMBINED-NEXT:    <VALUE_GUID
 ; COMBINED-NEXT:    <VALUE_GUID
 ; COMBINED-NEXT:    <VALUE_GUID
+; COMBINED-NEXT:    <VALUE_GUID
+; COMBINED-NEXT:    <VALUE_GUID
+; COMBINED-NEXT:    <COMBINED abbrevid=
 ; COMBINED-NEXT:    <COMBINED abbrevid=
 ; COMBINED-NEXT:    <COMBINED abbrevid=
 ; COMBINED-NEXT:    <COMBINED abbrevid=
 ; COMBINED-NEXT:    <COMBINED abbrevid=
 ; COMBINED-NEXT:    <COMBINED abbrevid=
 ; COMBINED-NEXT:    <COMBINED abbrevid=
-; COMBINED-NEXT:    <COMBINED_PROFILE {{.*}} op5=[[HOT1:.*]] op6=3 op7=[[COLD:.*]] op8=1 op9=[[HOT2:.*]] op10=3 op11=[[NONE1:.*]] op12=2 op13=[[HOT3:.*]] op14=3 op15=[[NONE2:.*]] op16=2 op17=[[NONE3:.*]] op18=2/>
+; COMBINED-NEXT:    <COMBINED abbrevid=
+; COMBINED-NEXT:    <COMBINED_PROFILE {{.*}} op5=[[NONE1:.*]] op6=1 op7=[[HOT1:.*]] op8=3 op9=[[COLD1:.*]] op10=1 op11=[[NONE2:.*]] op12=1 op13=[[HOT2:.*]] op14=3 op15=[[COLD2:.*]] op16=1 op17=[[NONE3:.*]] op18=1 op19=[[HOT3:.*]] op20=3 op21=[[COLD3:.*]] op22=1/>
 ; COMBINED_NEXT:    <COMBINED abbrevid=
 ; COMBINED_NEXT:  </GLOBALVAL_SUMMARY_BLOCK>
 
 
 ; ModuleID = 'thinlto-function-summary-callgraph.ll'
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; This function have high profile count, so entry block is hot.
 define void @hot_function(i1 %a, i1 %a2) !prof !20 {
 entry:
-    call void @hot1()
-    br i1 %a, label %Cold, label %Hot, !prof !41
+  call void @none1()
+  call void @hot1(), !prof !15
+  call void @cold1(), !prof !16
 Cold:           ; 1/1000 goes here
-  call void @cold()
-  call void @hot2()
-  call void @hot4(), !prof !15
-  call void @none1()
-  br label %exit
-Hot:            ; 999/1000 goes here
-  call void @hot2()
-  call void @hot3()
-  br i1 %a2, label %None1, label %None2, !prof !42
-None1:          ; half goes here
-  call void @none1()
   call void @none2()
+  call void @hot2(), !prof !15
+  call void @cold2(), !prof !16
   br label %exit
-None2:          ; half goes here
+Hot:            ; 999/1000 goes here
   call void @none3()
+  call void @hot3(), !prof !15
+  call void @cold3(), !prof !16
   br label %exit
 exit:
   ret void
 }
 
 declare void @hot1() #1
 declare void @hot2() #1
 declare void @hot3() #1
-declare void @hot4() #1
-declare void @cold() #1
+declare void @cold1() #1
+declare void @cold2() #1
+declare void @cold3() #1
 declare void @none1() #1
 declare void @none2() #1
 declare void @none3() #1
 
-
 !41 = !{!"branch_weights", i32 1, i32 1000}
-!42 = !{!"branch_weights", i32 1, i32 1}
-
-
 
 !llvm.module.flags = !{!1}
 !20 = !{!"function_entry_count", i64 110, i64 123}
 
 !1 = !{i32 1, !"ProfileSummary", !2}
 !2 = !{!3, !4, !5, !6, !7, !8, !9, !10}
 !3 = !{!"ProfileFormat", !"SampleProfile"}
 !4 = !{!"TotalCount", i64 10000}
 !5 = !{!"MaxCount", i64 10}
 !6 = !{!"MaxInternalCount", i64 1}
 !7 = !{!"MaxFunctionCount", i64 1000}
 !8 = !{!"NumCounts", i64 3}
 !9 = !{!"NumFunctions", i64 3}
 !10 = !{!"DetailedSummary", !11}
 !11 = !{!12, !13, !14}
 !12 = !{i32 10000, i64 100, i32 1}
 !13 = !{i32 999000, i64 100, i32 1}
 !14 = !{i32 999999, i64 1, i32 2}
 !15 = !{!"branch_weights", i32 100}
+!16 = !{!"branch_weights", i32 1}

unittests/Analysis/ProfileSummaryInfoTest.cpp

TEST_F(ProfileSummaryInfoTest, SampleProf) {
   BlockFrequencyInfo BFI = buildBFI(*F);
   EXPECT_TRUE(PSI.isHotBB(&BB0, &BFI));
   EXPECT_TRUE(PSI.isHotBB(BB1, &BFI));
   EXPECT_FALSE(PSI.isHotBB(BB2, &BFI));
   EXPECT_TRUE(PSI.isHotBB(BB3, &BFI));
 
   CallSite CS1(BB1->getFirstNonPHI());
   auto *CI2 = BB2->getFirstNonPHI();
+  // Manually attach branch weights metadata to the call instruction.
+  SmallVector<uint32_t, 1> Weights;
+  Weights.push_back(1000);
+  MDBuilder MDB(M->getContext());
+  CI2->setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(Weights));
   CallSite CS2(CI2);
 
-  EXPECT_TRUE(PSI.isHotCallSite(CS1, &BFI));
-  EXPECT_FALSE(PSI.isHotCallSite(CS2, &BFI));
+  EXPECT_FALSE(PSI.isHotCallSite(CS1, &BFI));
+  EXPECT_TRUE(PSI.isHotCallSite(CS2, &BFI));
 
   // Test that CS2 is considered hot when it gets an MD_prof metadata with
   // weights that exceed the hot count threshold.
-  MDBuilder MDB(M->getContext());
   CI2->setMetadata(llvm::LLVMContext::MD_prof, MDB.createBranchWeights({400}));
   EXPECT_TRUE(PSI.isHotCallSite(CS2, &BFI));
 }
 
 } // end anonymous namespace
 } // end namespace llvm