[llvm] [SHT_LLVM_BB_ADDR_MAP] Fixes two bugs in decoding of PGOAnalyses in BBAddrMap. (PR #77139)
Micah Weston via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 5 13:23:48 PST 2024
https://github.com/red1bluelost created https://github.com/llvm/llvm-project/pull/77139
We had specified that `readBBAddrMap` will always keep PGOAnalyses and BBAddrMaps the same length on success.
https://github.com/llvm/llvm-project/blob/365fbbfbcfefb8766f7716109b9c3767b58e6058/llvm/include/llvm/Object/ELFObjectFile.h#L116-L117
It turns out that this is not currently the case when no analyses exist in a function. No test had caught it.
We also should not append PGOBBEntries when there is no BBFreq or BrProb.
This patch adds:
* tests that PGOAnalyses and BBAddrMaps are same length even when no analyses are enabled
* fixes decode so that PGOAnalyses and BBAddrMaps are same length
* updates test to not emit unnecessary PGOBBEntries
* fixes decode to not emit PGOBBEntries when unnecessary
>From e84b80d9cb32c614ac150b03b46cc81920e6650d Mon Sep 17 00:00:00 2001
From: Micah Weston <micahsweston at gmail.com>
Date: Fri, 5 Jan 2024 16:12:01 -0500
Subject: [PATCH] [SHT_LLVM_BB_ADDR_MAP] Ensures that decoding keeps
PGOAnalysisMaps and BBAddrMaps same length.
---
llvm/include/llvm/Object/ELFTypes.h | 3 ++
llvm/lib/Object/ELF.cpp | 7 ++--
llvm/unittests/Object/ELFObjectFileTest.cpp | 37 ++++++++++++++++-----
3 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/llvm/include/llvm/Object/ELFTypes.h b/llvm/include/llvm/Object/ELFTypes.h
index d3351a2d1650ed..956f7811dd6c34 100644
--- a/llvm/include/llvm/Object/ELFTypes.h
+++ b/llvm/include/llvm/Object/ELFTypes.h
@@ -885,6 +885,9 @@ struct PGOAnalysisMap {
bool BBFreq : 1;
bool BrProb : 1;
+ // True if at least one feature is enabled
+ bool anyEnabled() const { return FuncEntryCount || BBFreq || BrProb; }
+
// Encodes to minimum bit width representation.
uint8_t encode() const {
return (static_cast<uint8_t>(FuncEntryCount) << 0) |
diff --git a/llvm/lib/Object/ELF.cpp b/llvm/lib/Object/ELF.cpp
index 300639f2bfa0af..f24395b02043db 100644
--- a/llvm/lib/Object/ELF.cpp
+++ b/llvm/lib/Object/ELF.cpp
@@ -774,7 +774,7 @@ decodeBBAddrMapImpl(const ELFFile<ELFT> &EF,
}
FunctionEntries.emplace_back(Address, std::move(BBEntries));
- if (FeatEnable.FuncEntryCount || FeatEnable.BBFreq || FeatEnable.BrProb) {
+ if (PGOAnalyses || FeatEnable.anyEnabled()) {
// Function entry count
uint64_t FuncEntryCount =
FeatEnable.FuncEntryCount
@@ -782,8 +782,9 @@ decodeBBAddrMapImpl(const ELFFile<ELFT> &EF,
: 0;
std::vector<PGOAnalysisMap::PGOBBEntry> PGOBBEntries;
- for (uint32_t BlockIndex = 0; !MetadataDecodeErr && !ULEBSizeErr && Cur &&
- (BlockIndex < NumBlocks);
+ for (uint32_t BlockIndex = 0;
+ (FeatEnable.BBFreq || FeatEnable.BrProb) && !MetadataDecodeErr &&
+ !ULEBSizeErr && Cur && (BlockIndex < NumBlocks);
++BlockIndex) {
// Block frequency
uint64_t BBF = FeatEnable.BBFreq
diff --git a/llvm/unittests/Object/ELFObjectFileTest.cpp b/llvm/unittests/Object/ELFObjectFileTest.cpp
index 2878ca088cd79d..502db0862ab8d8 100644
--- a/llvm/unittests/Object/ELFObjectFileTest.cpp
+++ b/llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -984,10 +984,23 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
BrProb: 0xffffffff
- BBFreq: 1000
Successors: []
-)");
+ - Name: .llvm_bb_addr_map_5
+ Type: SHT_LLVM_BB_ADDR_MAP
+ # Link: 0 (by default, can be overriden)
+ Entries:
+ - Version: 2
+ Address: 0x55555
+ Feature: 0x0
+ BBEntries:
+ - ID: 2
+ AddressOffset: 0x0
+ Size: 0x2
+ Metadata: 0x4
+ PGOAnalyses: [{}]
+ )");
BBAddrMap E1(0x11111, {{1, 0x0, 0x1, {false, true, false, false, false}}});
- PGOAnalysisMap P1 = {892, {{}}, {true, false, false}};
+ PGOAnalysisMap P1 = {892, {}, {true, false, false}};
BBAddrMap E2(0x22222, {{2, 0x0, 0x2, {false, false, true, false, false}}});
PGOAnalysisMap P2 = {{}, {{BlockFrequency(343), {}}}, {false, true, false}};
BBAddrMap E3(0x33333, {{0, 0x0, 0x3, {false, true, true, false, false}},
@@ -1016,16 +1029,18 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
{BlockFrequency(18), {{3, BranchProbability::getRaw(0xffff'ffff)}}},
{BlockFrequency(1000), {}}},
{true, true, true}};
+ BBAddrMap E5(0x55555, {{2, 0x0, 0x2, {false, false, true, false, false}}});
+ PGOAnalysisMap P5 = {{}, {}, {false, false, false}};
- std::vector<BBAddrMap> Section0BBAddrMaps = {E4};
+ std::vector<BBAddrMap> Section0BBAddrMaps = {E4, E5};
std::vector<BBAddrMap> Section1BBAddrMaps = {E3};
std::vector<BBAddrMap> Section2BBAddrMaps = {E1, E2};
- std::vector<BBAddrMap> AllBBAddrMaps = {E1, E2, E3, E4};
+ std::vector<BBAddrMap> AllBBAddrMaps = {E1, E2, E3, E4, E5};
- std::vector<PGOAnalysisMap> Section0PGOAnalysisMaps = {P4};
+ std::vector<PGOAnalysisMap> Section0PGOAnalysisMaps = {P4, P5};
std::vector<PGOAnalysisMap> Section1PGOAnalysisMaps = {P3};
std::vector<PGOAnalysisMap> Section2PGOAnalysisMaps = {P1, P2};
- std::vector<PGOAnalysisMap> AllPGOAnalysisMaps = {P1, P2, P3, P4};
+ std::vector<PGOAnalysisMap> AllPGOAnalysisMaps = {P1, P2, P3, P4, P5};
auto DoCheckSucceeds =
[&](StringRef YamlString, std::optional<unsigned> TextSectionIndex,
@@ -1048,6 +1063,10 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
if (ExpectedPGO) {
EXPECT_EQ(BBAddrMaps->size(), PGOAnalyses.size());
EXPECT_EQ(PGOAnalyses, *ExpectedPGO);
+ for (auto &&[BB, PGO] : llvm::zip(*BBAddrMaps, PGOAnalyses)) {
+ if (PGO.FeatEnable.BBFreq || PGO.FeatEnable.BrProb)
+ EXPECT_EQ(BB.getBBEntries().size(), PGO.BBEntries.size());
+ }
}
};
@@ -1099,9 +1118,9 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
Link: 10
)";
- DoCheckFails(InvalidLinkedYamlString, /*TextSectionIndex=*/4,
+ DoCheckFails(InvalidLinkedYamlString, /*TextSectionIndex=*/5,
"unable to get the linked-to section for "
- "SHT_LLVM_BB_ADDR_MAP section with index 4: invalid section "
+ "SHT_LLVM_BB_ADDR_MAP section with index 5: invalid section "
"index: 10");
// Linked sections are not checked when we don't target a specific text
// section.
@@ -1117,7 +1136,7 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
)";
DoCheckFails(TruncatedYamlString, /*TextSectionIndex=*/std::nullopt,
- "unable to read SHT_LLVM_BB_ADDR_MAP section with index 4: "
+ "unable to read SHT_LLVM_BB_ADDR_MAP section with index 5: "
"unable to decode LEB128 at offset 0x0000000a: malformed "
"uleb128, extends past end");
// Check that we can read the other section's bb-address-maps which are
More information about the llvm-commits
mailing list