Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AsmPrint] Dump raw frequencies in -mbb-profile-dump #66818

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Conversation

mtrofin
Copy link
Member

@mtrofin mtrofin commented Sep 19, 2023

We were losing the function entry count, which is useful to check
profile quality. For the original cases where we want
entrypoint-relative MBB frequencies, the user would just need to divide
these values by the entrypoint (first MBB, with ID=0) value.

We were losing the function entry count, which is useful to check
profile quality. For the original cases where we want
entrypoint-relative MBB frequencies, the user would just need to divide
these values by the entrypoint (first MBB, with ID=0) value.
@llvmbot llvmbot added the mlgo label Sep 19, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 19, 2023

@llvm/pr-subscribers-mlgo

Changes

We were losing the function entry count, which is useful to check
profile quality. For the original cases where we want
entrypoint-relative MBB frequencies, the user would just need to divide
these values by the entrypoint (first MBB, with ID=0) value.


Full diff: https://github.com/llvm/llvm-project/pull/66818.diff

2 Files Affected:

  • (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (+1-1)
  • (modified) llvm/test/CodeGen/MLRegAlloc/bb-profile-dump.ll (+4-4)
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 2ce08a2ff43955b..0c4ea1b3d9f04d9 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1940,7 +1940,7 @@ void AsmPrinter::emitFunctionBody() {
     for (const auto &MBB : *MF) {
       *MBBProfileDumpFileOutput.get()
           << MF->getName() << "," << MBB.getBBID() << ","
-          << MBFI.getBlockFreqRelativeToEntryBlock(&MBB) << "\n";
+          << MBFI.getBlockFreq(&MBB).getFrequency() << "\n";
     }
   }
 }
diff --git a/llvm/test/CodeGen/MLRegAlloc/bb-profile-dump.ll b/llvm/test/CodeGen/MLRegAlloc/bb-profile-dump.ll
index e0ac148456cacbb..60a1fafc3c92c80 100644
--- a/llvm/test/CodeGen/MLRegAlloc/bb-profile-dump.ll
+++ b/llvm/test/CodeGen/MLRegAlloc/bb-profile-dump.ll
@@ -22,10 +22,10 @@ ifNotEqual:
     ret i64 %sum
 }
 
-; CHECK: f2,0,1.000000e+00
-; CHECK-NEXT: f1,0,1.000000e+00
-; CHECK-NEXT: f1,1,5.000000e-01
-; CHECK-NEXT: f1,2,1.000000e+00
+; CHECK: f2,0,8
+; CHECK-NEXT: f1,0,16
+; CHECK-NEXT: f1,1,8
+; CHECK-NEXT: f1,2,16
 
 ; Check that if we pass -mbb-profile-dump but don't set -basic-block-sections,
 ; we get an appropriate error message

@mtrofin
Copy link
Member Author

mtrofin commented Sep 19, 2023

Side note, maybe we should move this test in ../Generic/ in a subsequent patch.

Copy link
Contributor

@snehasish snehasish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a minute to realize that f2 and f1 were normalized separate with 8 and 16 previously. Perhaps add a comment to clarify that in the test.

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Even in downstream applications like llvm-cm it probably makes more sense to do this than using the relative block frequency.

I can push a commit to move the test once this lands. Definitely doesn't make a lot of sense to keep it in CodeGen/MLRegAlloc when it's not specific to the register allocator at all.

@mtrofin mtrofin merged commit d8873df into llvm:main Sep 19, 2023
@mtrofin mtrofin deleted the mbbfreq branch September 19, 2023 21:37
mtrofin added a commit that referenced this pull request Sep 30, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…quencies (#67826)

The goal in #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.

We also increase the precision of `MachineBlockFrequencyInfo::getBlockFreqRelativeToEntryBlock` to double. Existing code uses it as float so should be unaffected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants