From 4c11591a42d2132589b886ade415bc1ed2dc542b Mon Sep 17 00:00:00 2001 From: Mircea Trofin Date: Fri, 29 Sep 2023 09:06:12 -0700 Subject: [PATCH 1/3] [AsmPrint] Correctly factor function entry count when dumping MBB frequencies The goal in PR #66818 was to capture function entry counts, but those are not the same as the frequency of the entry (machine) basic block. This fixes that, and adds explicit profiles to the test. --- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 23 +++++++++++-- llvm/test/CodeGen/Generic/bb-profile-dump.ll | 36 ++++++++++++++++---- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp index 0c4ea1b3d9f04..a54a5ab75797b 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -1929,7 +1929,7 @@ void AsmPrinter::emitFunctionBody() { // Output MBB ids, function names, and frequencies if the flag to dump // MBB profile information has been set - if (MBBProfileDumpFileOutput) { + if (MBBProfileDumpFileOutput && !MF->empty()) { if (!MF->hasBBLabels()) MF->getContext().reportError( SMLoc(), @@ -1937,10 +1937,27 @@ void AsmPrinter::emitFunctionBody() { "must be called with -basic-block-sections=labels"); MachineBlockFrequencyInfo &MBFI = getAnalysis().getBFI(); + // The entry count and the entry basic block frequency aren't the same. We + // want to capture "absolute" frequencies, i.e. the frequency with which a + // MBB is executed when the program is executed - from there, we can derive + // Function-relative frequencies (divide by the value for the first MBB), + // and we also have the information about frequency with which functions + // were called. This helps, for example, in a type of integration tests + // where we want to cross-validate the compiler's profile with a real + // profile. + // Using double precision because uint64 values used to encode mbb + // "frequencies" may be quite large. + const double EntryCount = + static_cast(MF->getFunction().getEntryCount()->getCount()); + const double EntryFrequency = + static_cast(MBFI.getBlockFreq(&*MF->begin()).getFrequency()); + const double FreqAdjustmentFactor = EntryCount / EntryFrequency; for (const auto &MBB : *MF) { + const double MBBFreq = + static_cast(MBFI.getBlockFreq(&MBB).getFrequency()); + const double AbsMBBFreq = MBBFreq * FreqAdjustmentFactor; *MBBProfileDumpFileOutput.get() - << MF->getName() << "," << MBB.getBBID() << "," - << MBFI.getBlockFreq(&MBB).getFrequency() << "\n"; + << MF->getName() << "," << MBB.getBBID() << "," << AbsMBBFreq << "\n"; } } } diff --git a/llvm/test/CodeGen/Generic/bb-profile-dump.ll b/llvm/test/CodeGen/Generic/bb-profile-dump.ll index 934c281219d4a..7391a6ee6f912 100644 --- a/llvm/test/CodeGen/Generic/bb-profile-dump.ll +++ b/llvm/test/CodeGen/Generic/bb-profile-dump.ll @@ -7,26 +7,48 @@ ; Check that given a simple case, we can return the default MBFI -define i64 @f2(i64 %a, i64 %b) { +define i64 @f2(i64 %a, i64 %b) !prof !1{ %sum = add i64 %a, %b ret i64 %sum } -; CHECK: f2,0,8 +; CHECK: f2,0,1.000000e+03 -define i64 @f1() { +define i64 @f1() !prof !2{ %sum = call i64 @f2(i64 2, i64 2) %isEqual = icmp eq i64 %sum, 4 - br i1 %isEqual, label %ifEqual, label %ifNotEqual + br i1 %isEqual, label %ifEqual, label %ifNotEqual, !prof !3 ifEqual: ret i64 0 ifNotEqual: ret i64 %sum } -; CHECK-NEXT: f1,0,16 -; CHECK-NEXT: f1,1,8 -; CHECK-NEXT: f1,2,16 +; CHECK-NEXT: f1,0,1.000000e+01 +; CHECK-NEXT: f1,2,6.000000e+00 +; CHECK-NEXT: f1,1,4.000000e+00 + +define void @f3(i32 %iter) !prof !4 { +entry: + br label %loop +loop: + %i = phi i32 [0, %entry], [%i_next, %loop] + %i_next = add i32 %i, 1 + %exit_cond = icmp slt i32 %i_next, %iter + br i1 %exit_cond, label %loop, label %exit, !prof !5 +exit: + ret void +} + +; CHECK-NEXT: f3,0,2.000000e+00 +; CHECK-NEXT: f3,1,2.002000e+03 +; CHECK-NEXT: f3,2,2.000000e+00 + +!1 = !{!"function_entry_count", i64 1000} +!2 = !{!"function_entry_count", i64 10} +!3 = !{!"branch_weights", i32 2, i32 3} +!4 = !{!"function_entry_count", i64 2} +!5 = !{!"branch_weights", i32 1000, i32 1} ; Check that if we pass -mbb-profile-dump but don't set -basic-block-sections, ; we get an appropriate error message From 39bcca3f0336c8bfb850298b2c89f818c8d8b68a Mon Sep 17 00:00:00 2001 From: Mircea Trofin Date: Fri, 29 Sep 2023 10:18:35 -0700 Subject: [PATCH 2/3] Split long sentences in comment. --- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp index a54a5ab75797b..5b1f7adf78eaf 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -1939,9 +1939,9 @@ void AsmPrinter::emitFunctionBody() { getAnalysis().getBFI(); // The entry count and the entry basic block frequency aren't the same. We // want to capture "absolute" frequencies, i.e. the frequency with which a - // MBB is executed when the program is executed - from there, we can derive - // Function-relative frequencies (divide by the value for the first MBB), - // and we also have the information about frequency with which functions + // MBB is executed when the program is executed. From there, we can derive + // Function-relative frequencies (divide by the value for the first MBB). + // We also have the information about frequency with which functions // were called. This helps, for example, in a type of integration tests // where we want to cross-validate the compiler's profile with a real // profile. From 05dc741c4959bc59b65725014425ba9b5fac74ae Mon Sep 17 00:00:00 2001 From: Mircea Trofin Date: Fri, 29 Sep 2023 11:55:55 -0700 Subject: [PATCH 3/3] feedback --- llvm/include/llvm/CodeGen/MachineBlockFrequencyInfo.h | 5 +++-- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp | 11 ++++------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/llvm/include/llvm/CodeGen/MachineBlockFrequencyInfo.h b/llvm/include/llvm/CodeGen/MachineBlockFrequencyInfo.h index 6d58c7a14fb95..1152eefed6e45 100644 --- a/llvm/include/llvm/CodeGen/MachineBlockFrequencyInfo.h +++ b/llvm/include/llvm/CodeGen/MachineBlockFrequencyInfo.h @@ -65,9 +65,10 @@ class MachineBlockFrequencyInfo : public MachineFunctionPass { /// Compute the frequency of the block, relative to the entry block. /// This API assumes getEntryFreq() is non-zero. - float getBlockFreqRelativeToEntryBlock(const MachineBasicBlock *MBB) const { + double getBlockFreqRelativeToEntryBlock(const MachineBasicBlock *MBB) const { assert(getEntryFreq() != 0 && "getEntryFreq() should not return 0 here!"); - return getBlockFreq(MBB).getFrequency() * (1.0f / getEntryFreq()); + return static_cast(getBlockFreq(MBB).getFrequency()) / + static_cast(getEntryFreq()); } std::optional diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp index 5b1f7adf78eaf..97d2fe3426406 100644 --- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp +++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp @@ -1929,7 +1929,8 @@ void AsmPrinter::emitFunctionBody() { // Output MBB ids, function names, and frequencies if the flag to dump // MBB profile information has been set - if (MBBProfileDumpFileOutput && !MF->empty()) { + if (MBBProfileDumpFileOutput && !MF->empty() && + MF->getFunction().getEntryCount()) { if (!MF->hasBBLabels()) MF->getContext().reportError( SMLoc(), @@ -1949,13 +1950,9 @@ void AsmPrinter::emitFunctionBody() { // "frequencies" may be quite large. const double EntryCount = static_cast(MF->getFunction().getEntryCount()->getCount()); - const double EntryFrequency = - static_cast(MBFI.getBlockFreq(&*MF->begin()).getFrequency()); - const double FreqAdjustmentFactor = EntryCount / EntryFrequency; for (const auto &MBB : *MF) { - const double MBBFreq = - static_cast(MBFI.getBlockFreq(&MBB).getFrequency()); - const double AbsMBBFreq = MBBFreq * FreqAdjustmentFactor; + const double MBBRelFreq = MBFI.getBlockFreqRelativeToEntryBlock(&MBB); + const double AbsMBBFreq = MBBRelFreq * EntryCount; *MBBProfileDumpFileOutput.get() << MF->getName() << "," << MBB.getBBID() << "," << AbsMBBFreq << "\n"; }