-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[NFC][StaticDataProfileInfo] Refactor StaticDataProfileInfo::getConstantSectionPrefix and extract analysis based on PGO-counter to be a helper function #162388
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
Conversation
8c64a53
to
3d70a00
Compare
@llvm/pr-subscribers-llvm-analysis Author: Mingming Liu (mingmingl-llvm) Changes
Before this patch, its implementation does analysis using PGO-counters, and PGO-counters are only available on module-internal constants. After this patch, the PGO-counter analysis are extracted to a helper function, and returns enum rather than StringPrefix. This way, the follow up patch #163325 can extend this function to use global variable section prefix and compute a max (the hotter one). [1] llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp Lines 3014 to 3019 in 975fba1
[2] llvm-project/llvm/lib/CodeGen/StaticDataAnnotator.cpp Lines 77 to 84 in 975fba1
Full diff: https://github.com/llvm/llvm-project/pull/162388.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Analysis/StaticDataProfileInfo.h b/llvm/include/llvm/Analysis/StaticDataProfileInfo.h
index f06e7ceaa74ce..70199a904f320 100644
--- a/llvm/include/llvm/Analysis/StaticDataProfileInfo.h
+++ b/llvm/include/llvm/Analysis/StaticDataProfileInfo.h
@@ -32,8 +32,11 @@ bool IsAnnotationOK(const GlobalVariable &GV);
/// profile information and provides methods to operate on them.
class StaticDataProfileInfo {
public:
- /// Accummulate the profile count of a constant that will be lowered to static
- /// data sections.
+ /// A constant is tracked only if the following conditions are met.
+ /// 1) It has local (i.e., private or internal) linkage.
+ // 2) Its data kind is one of {.rodata, .data, .bss, .data.rel.ro}.
+ // 3) It's eligible for section prefix annotation. See `AnnotationKind`
+ // above for ineligible reasons.
DenseMap<const Constant *, uint64_t> ConstantProfileCounts;
/// Keeps track of the constants that are seen at least once without profile
@@ -44,6 +47,20 @@ class StaticDataProfileInfo {
LLVM_ABI std::optional<uint64_t>
getConstantProfileCount(const Constant *C) const;
+ enum class StaticDataHotness : uint8_t {
+ Cold = 0,
+ LukewarmOrUnknown = 1,
+ Hot = 2,
+ };
+
+ /// Return the hotness of the constant \p C based on its profile count \p
+ /// Count.
+ LLVM_ABI StaticDataHotness getSectionHotnessUsingProfileCount(
+ const Constant *C, const ProfileSummaryInfo *PSI, uint64_t Count) const;
+
+ /// Return the string representation of the hotness enum \p Hotness.
+ LLVM_ABI StringRef hotnessToStr(StaticDataHotness Hotness) const;
+
public:
StaticDataProfileInfo() = default;
diff --git a/llvm/lib/Analysis/StaticDataProfileInfo.cpp b/llvm/lib/Analysis/StaticDataProfileInfo.cpp
index 1f751ee5e09d9..27f8d216454aa 100644
--- a/llvm/lib/Analysis/StaticDataProfileInfo.cpp
+++ b/llvm/lib/Analysis/StaticDataProfileInfo.cpp
@@ -60,6 +60,37 @@ void StaticDataProfileInfo::addConstantProfileCount(
OriginalCount = getInstrMaxCountValue();
}
+StaticDataProfileInfo::StaticDataHotness
+StaticDataProfileInfo::getSectionHotnessUsingProfileCount(
+ const Constant *C, const ProfileSummaryInfo *PSI, uint64_t Count) const {
+ // The accummulated counter shows the constant is hot. Return 'hot' whether
+ // this variable is seen by unprofiled functions or not.
+ if (PSI->isHotCount(Count))
+ return StaticDataHotness::Hot;
+ // The constant is not hot, and seen by unprofiled functions. We don't want to
+ // assign it to unlikely sections, even if the counter says 'cold'. So return
+ // an empty prefix before checking whether the counter is cold.
+ if (ConstantWithoutCounts.count(C))
+ return StaticDataHotness::LukewarmOrUnknown;
+ // The accummulated counter shows the constant is cold. Return 'unlikely'.
+ if (PSI->isColdCount(Count))
+ return StaticDataHotness::Cold;
+
+ return StaticDataHotness::LukewarmOrUnknown;
+}
+
+StringRef StaticDataProfileInfo::hotnessToStr(
+ StaticDataProfileInfo::StaticDataHotness Hotness) const {
+ switch (Hotness) {
+ case StaticDataProfileInfo::StaticDataHotness::Cold:
+ return "unlikely";
+ case StaticDataProfileInfo::StaticDataHotness::Hot:
+ return "hot";
+ default:
+ return "";
+ }
+}
+
std::optional<uint64_t>
StaticDataProfileInfo::getConstantProfileCount(const Constant *C) const {
auto I = ConstantProfileCounts.find(C);
@@ -70,23 +101,10 @@ StaticDataProfileInfo::getConstantProfileCount(const Constant *C) const {
StringRef StaticDataProfileInfo::getConstantSectionPrefix(
const Constant *C, const ProfileSummaryInfo *PSI) const {
- auto Count = getConstantProfileCount(C);
+ std::optional<uint64_t> Count = getConstantProfileCount(C);
if (!Count)
return "";
- // The accummulated counter shows the constant is hot. Return 'hot' whether
- // this variable is seen by unprofiled functions or not.
- if (PSI->isHotCount(*Count))
- return "hot";
- // The constant is not hot, and seen by unprofiled functions. We don't want to
- // assign it to unlikely sections, even if the counter says 'cold'. So return
- // an empty prefix before checking whether the counter is cold.
- if (ConstantWithoutCounts.count(C))
- return "";
- // The accummulated counter shows the constant is cold. Return 'unlikely'.
- if (PSI->isColdCount(*Count))
- return "unlikely";
- // The counter says lukewarm. Return an empty prefix.
- return "";
+ return hotnessToStr(getSectionHotnessUsingProfileCount(C, PSI, *Count));
}
bool StaticDataProfileInfoWrapperPass::doInitialization(Module &M) {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with some minor nits.
|
||
enum class StaticDataHotness : uint8_t { | ||
Cold = 0, | ||
LukewarmOrUnknown = 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer unknown as 0 so that any accidentally uninitialized value will default to unknown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meanwhile updated the enum class def to be signed enums.
|
||
/// Return the hotness of the constant \p C based on its profile count \p | ||
/// Count. | ||
LLVM_ABI StaticDataHotness getSectionHotnessUsingProfileCount( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name this getConstantHotnessUsingProfileCount
? I wasn't sure what is implied by the word section in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
return StaticDataHotness::Hot; | ||
// The constant is not hot, and seen by unprofiled functions. We don't want to | ||
// assign it to unlikely sections, even if the counter says 'cold'. So return | ||
// an empty prefix before checking whether the counter is cold. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this returns an enum now, same for the comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
StringRef StaticDataProfileInfo::hotnessToStr( | ||
StaticDataProfileInfo::StaticDataHotness Hotness) const { | ||
switch (Hotness) { | ||
case StaticDataProfileInfo::StaticDataHotness::Cold: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the StaticDataProfileInfo
qualifier needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch! I removed the qualifier to make code simpler.
…ata access profiles (#163325) This PR enhances the `StaticDataProfileInfo::getConstantSectionPrefix` pass to reconcile data hotness information from both PGO counters and data access profiles. When both profiles are available for a global variable, the pass will now use the "hotter" of the two to determine the variable's section placement. This is a follow-up patch of #162388
…nters and data access profiles (#163325) This PR enhances the `StaticDataProfileInfo::getConstantSectionPrefix` pass to reconcile data hotness information from both PGO counters and data access profiles. When both profiles are available for a global variable, the pass will now use the "hotter" of the two to determine the variable's section placement. This is a follow-up patch of llvm/llvm-project#162388
StaticDataProfileInfo::getConstantSectionPrefix
is used twice in codegen ([1] and [2]) to emit section prefix for constants.Before this patch, its implementation does analysis using PGO-counters, and PGO-counters are only available on module-internal constants.
After this patch, the PGO-counter analysis are extracted to a helper function, and returns enum rather than StringPrefix. This way, the follow up patch #163325 can extend this function to use global variable section prefix and compute a max (the hotter one).
[1]
llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
Lines 3014 to 3019 in 975fba1
[2]
llvm-project/llvm/lib/CodeGen/StaticDataAnnotator.cpp
Lines 77 to 84 in 975fba1