[SHT_LLVM_BB_ADDR_MAP] Add an option to skip emitting bb entries #114447
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wlei-llvm
requested review from
MatzeB,
WenleiHe,
boomanaiden154,
red1bluelost and
rlavaee
Member
|
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-backend-x86 Author: Lei Wang (wlei-llvm) ChangesSometimes we want to use a Note that this naturally supports backwards compatibility as the field is zero for the old version, matches the decoding in the new version llvm. Full diff: https://github.com/llvm/llvm-project/pull/114447.diff 9 Files Affected:
diff --git a/llvm/include/llvm/Object/ELFTypes.h b/llvm/include/llvm/Object/ELFTypes.h
index 0f8c73f81cfa6d..ca05ab7e0b1fb7 100644
--- a/llvm/include/llvm/Object/ELFTypes.h
+++ b/llvm/include/llvm/Object/ELFTypes.h
@@ -830,6 +830,7 @@ struct BBAddrMap {
bool BBFreq : 1;
bool BrProb : 1;
bool MultiBBRange : 1;
+ bool NoBBEntries : 1;
bool hasPGOAnalysis() const { return FuncEntryCount || BBFreq || BrProb; }
@@ -840,7 +841,8 @@ struct BBAddrMap {
return (static_cast<uint8_t>(FuncEntryCount) << 0) |
(static_cast<uint8_t>(BBFreq) << 1) |
(static_cast<uint8_t>(BrProb) << 2) |
- (static_cast<uint8_t>(MultiBBRange) << 3);
+ (static_cast<uint8_t>(MultiBBRange) << 3) |
+ (static_cast<uint8_t>(NoBBEntries) << 4);
}
// Decodes from minimum bit width representation and validates no
@@ -848,7 +850,8 @@ struct BBAddrMap {
static Expected<Features> decode(uint8_t Val) {
Features Feat{
static_cast<bool>(Val & (1 << 0)), static_cast<bool>(Val & (1 << 1)),
- static_cast<bool>(Val & (1 << 2)), static_cast<bool>(Val & (1 << 3))};
+ static_cast<bool>(Val & (1 << 2)), static_cast<bool>(Val & (1 << 3)),
+ static_cast<bool>(Val & (1 << 4))};
if (Feat.encode() != Val)
return createStringError(
std::error_code(), "invalid encoding for BBAddrMap::Features: 0x%x",
@@ -857,9 +860,10 @@ struct BBAddrMap {
}
bool operator==(const Features &Other) const {
- return std::tie(FuncEntryCount, BBFreq, BrProb, MultiBBRange) ==
+ return std::tie(FuncEntryCount, BBFreq, BrProb, MultiBBRange,
+ NoBBEntries) ==
std::tie(Other.FuncEntryCount, Other.BBFreq, Other.BrProb,
- Other.MultiBBRange);
+ Other.MultiBBRange, Other.NoBBEntries);
}
};
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 459ad15163ae5e..960c9e3252d1d9 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -161,6 +161,12 @@ static cl::bits<PGOMapFeaturesEnum> PgoAnalysisMapFeatures(
"Enable extended information within the SHT_LLVM_BB_ADDR_MAP that is "
"extracted from PGO related analysis."));
+static cl::opt<bool>
+ SkipEmitBBEntries("skip-emit-bb-entries",
+ cl::desc("Skip emitting basic block entries in the "
+ "SHT_LLVM_BB_ADDR_MAP section"),
+ cl::Hidden, cl::init(false));
+
static cl::opt<bool> EmitJumpTableSizesSection(
"emit-jump-table-sizes-section",
cl::desc("Emit a section containing jump table addresses and sizes"),
@@ -1392,8 +1398,14 @@ getBBAddrMapFeature(const MachineFunction &MF, int NumMBBSectionRanges) {
bool BrProbEnabled =
AllFeatures ||
(!NoFeatures && PgoAnalysisMapFeatures.isSet(PGOMapFeaturesEnum::BrProb));
+
+ if ((BBFreqEnabled || BrProbEnabled) && SkipEmitBBEntries) {
+ MF.getFunction().getContext().emitError(
+ "BB entries info is required for BBFreq and BrProb "
+ "features");
+ }
return {FuncEntryCountEnabled, BBFreqEnabled, BrProbEnabled,
- MF.hasBBSections() && NumMBBSectionRanges > 1};
+ MF.hasBBSections() && NumMBBSectionRanges > 1, SkipEmitBBEntries};
}
void AsmPrinter::emitBBAddrMapSection(const MachineFunction &MF) {
@@ -1450,24 +1462,28 @@ void AsmPrinter::emitBBAddrMapSection(const MachineFunction &MF) {
OutStreamer->emitULEB128IntValue(MBBSectionNumBlocks[MBB.getSectionID()]);
PrevMBBEndSymbol = MBBSymbol;
}
- // TODO: Remove this check when version 1 is deprecated.
- if (BBAddrMapVersion > 1) {
- OutStreamer->AddComment("BB id");
- // Emit the BB ID for this basic block.
- // We only emit BaseID since CloneID is unset for
- // -basic-block-adress-map.
- // TODO: Emit the full BBID when labels and sections can be mixed
- // together.
- OutStreamer->emitULEB128IntValue(MBB.getBBID()->BaseID);
+
+ if (!Features.NoBBEntries) {
+ // TODO: Remove this check when version 1 is deprecated.
+ if (BBAddrMapVersion > 1) {
+ OutStreamer->AddComment("BB id");
+ // Emit the BB ID for this basic block.
+ // We only emit BaseID since CloneID is unset for
+ // -basic-block-adress-map.
+ // TODO: Emit the full BBID when labels and sections can be mixed
+ // together.
+ OutStreamer->emitULEB128IntValue(MBB.getBBID()->BaseID);
+ }
+ // Emit the basic block offset relative to the end of the previous block.
+ // This is zero unless the block is padded due to alignment.
+ emitLabelDifferenceAsULEB128(MBBSymbol, PrevMBBEndSymbol);
+ // Emit the basic block size. When BBs have alignments, their size cannot
+ // always be computed from their offsets.
+ emitLabelDifferenceAsULEB128(MBB.getEndSymbol(), MBBSymbol);
+ // Emit the Metadata.
+ OutStreamer->emitULEB128IntValue(getBBAddrMapMetadata(MBB));
}
- // Emit the basic block offset relative to the end of the previous block.
- // This is zero unless the block is padded due to alignment.
- emitLabelDifferenceAsULEB128(MBBSymbol, PrevMBBEndSymbol);
- // Emit the basic block size. When BBs have alignments, their size cannot
- // always be computed from their offsets.
- emitLabelDifferenceAsULEB128(MBB.getEndSymbol(), MBBSymbol);
- // Emit the Metadata.
- OutStreamer->emitULEB128IntValue(getBBAddrMapMetadata(MBB));
+
PrevMBBEndSymbol = MBB.getEndSymbol();
}
diff --git a/llvm/lib/Object/ELF.cpp b/llvm/lib/Object/ELF.cpp
index 9a9e762d1e51d6..dafd1d4db267cf 100644
--- a/llvm/lib/Object/ELF.cpp
+++ b/llvm/lib/Object/ELF.cpp
@@ -824,6 +824,7 @@ decodeBBAddrMapImpl(const ELFFile<ELFT> &EF,
uint32_t NumBBRanges = 1;
typename ELFFile<ELFT>::uintX_t RangeBaseAddress = 0;
std::vector<BBAddrMap::BBEntry> BBEntries;
+ uint32_t TotalNumBlocks = 0;
if (FeatEnable.MultiBBRange) {
NumBBRanges = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
if (!Cur || ULEBSizeErr)
@@ -840,7 +841,6 @@ decodeBBAddrMapImpl(const ELFFile<ELFT> &EF,
NumBlocksInBBRange = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
}
std::vector<BBAddrMap::BBRangeEntry> BBRangeEntries;
- uint32_t TotalNumBlocks = 0;
for (uint32_t BBRangeIndex = 0; BBRangeIndex < NumBBRanges;
++BBRangeIndex) {
uint32_t PrevBBEndOffset = 0;
@@ -851,29 +851,33 @@ decodeBBAddrMapImpl(const ELFFile<ELFT> &EF,
RangeBaseAddress = *AddressOrErr;
NumBlocksInBBRange = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
}
- for (uint32_t BlockIndex = 0; !MetadataDecodeErr && !ULEBSizeErr && Cur &&
- (BlockIndex < NumBlocksInBBRange);
- ++BlockIndex) {
- uint32_t ID = Version >= 2
- ? readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr)
- : BlockIndex;
- uint32_t Offset = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
- uint32_t Size = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
- uint32_t MD = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
- if (Version >= 1) {
- // Offset is calculated relative to the end of the previous BB.
- Offset += PrevBBEndOffset;
- PrevBBEndOffset = Offset + Size;
- }
- Expected<BBAddrMap::BBEntry::Metadata> MetadataOrErr =
- BBAddrMap::BBEntry::Metadata::decode(MD);
- if (!MetadataOrErr) {
- MetadataDecodeErr = MetadataOrErr.takeError();
- break;
+
+ if (!FeatEnable.NoBBEntries) {
+
+ for (uint32_t BlockIndex = 0; !MetadataDecodeErr && !ULEBSizeErr &&
+ Cur && (BlockIndex < NumBlocksInBBRange);
+ ++BlockIndex) {
+ uint32_t ID = Version >= 2
+ ? readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr)
+ : BlockIndex;
+ uint32_t Offset = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
+ uint32_t Size = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
+ uint32_t MD = readULEB128As<uint32_t>(Data, Cur, ULEBSizeErr);
+ if (Version >= 1) {
+ // Offset is calculated relative to the end of the previous BB.
+ Offset += PrevBBEndOffset;
+ PrevBBEndOffset = Offset + Size;
+ }
+ Expected<BBAddrMap::BBEntry::Metadata> MetadataOrErr =
+ BBAddrMap::BBEntry::Metadata::decode(MD);
+ if (!MetadataOrErr) {
+ MetadataDecodeErr = MetadataOrErr.takeError();
+ break;
+ }
+ BBEntries.push_back({ID, Offset, Size, *MetadataOrErr});
}
- BBEntries.push_back({ID, Offset, Size, *MetadataOrErr});
+ TotalNumBlocks += BBEntries.size();
}
- TotalNumBlocks += BBEntries.size();
BBRangeEntries.push_back({RangeBaseAddress, std::move(BBEntries)});
}
FunctionEntries.push_back({std::move(BBRangeEntries)});
diff --git a/llvm/lib/ObjectYAML/ELFEmitter.cpp b/llvm/lib/ObjectYAML/ELFEmitter.cpp
index fc234581a45a70..13b3ed2cbfb419 100644
--- a/llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -1502,6 +1502,8 @@ void ELFState<ELFT>::writeSectionContent(
// Write all BBEntries in this BBRange.
if (!BBR.BBEntries)
continue;
+ if (FeatureOrErr->NoBBEntries)
+ continue;
for (const ELFYAML::BBAddrMapEntry::BBEntry &BBE : *BBR.BBEntries) {
++TotalNumBlocks;
if (Section.Type == llvm::ELF::SHT_LLVM_BB_ADDR_MAP && E.Version > 1)
diff --git a/llvm/test/CodeGen/X86/basic-block-address-map-pgo-features.ll b/llvm/test/CodeGen/X86/basic-block-address-map-pgo-features.ll
index fca5aa046b03b9..a7d7a4d465a35b 100644
--- a/llvm/test/CodeGen/X86/basic-block-address-map-pgo-features.ll
+++ b/llvm/test/CodeGen/X86/basic-block-address-map-pgo-features.ll
@@ -11,6 +11,10 @@
; RUN: llc < %s -mtriple=x86_64 -function-sections -unique-section-names=true -basic-block-address-map -pgo-analysis-map=bb-freq | FileCheck %s --check-prefixes=CHECK,PGO-BBF,BBF-ONLY
; RUN: llc < %s -mtriple=x86_64 -function-sections -unique-section-names=true -basic-block-address-map -pgo-analysis-map=br-prob | FileCheck %s --check-prefixes=CHECK,PGO-BRP,BRP-ONLY
+; RUN: llc < %s -mtriple=x86_64 -function-sections -unique-section-names=true -basic-block-address-map -pgo-analysis-map=func-entry-count -skip-emit-bb-entries | FileCheck %s --check-prefixes=SKIP-BB-ENTRIES
+; RUN: not llc < %s -mtriple=x86_64 -function-sections -unique-section-names=true -basic-block-address-map -pgo-analysis-map=bb-freq -skip-emit-bb-entries
+; RUN: not llc < %s -mtriple=x86_64 -function-sections -unique-section-names=true -basic-block-address-map -pgo-analysis-map=br-prob -skip-emit-bb-entries
+
;; Verify that we emit an error if we try and specify values in addition to all/none
; RUN: not llc < %s -mtriple=x86_64 -basic-block-address-map -pgo-analysis-map=all,bb-freq
; RUN: not llc < %s -mtriple=x86_64 -basic-block-address-map -pgo-analysis-map=none,bb-freq
@@ -134,3 +138,9 @@ declare i32 @__gxx_personality_v0(...)
; PGO-BRP-NEXT: .byte 5 # successor BB ID
; PGO-BRP-NEXT: .ascii "\200\200\200\200\b" # successor branch probability
+
+; SKIP-BB-ENTRIES: .byte 17 # feature
+; SKIP-BB-ENTRIES-NEXT: .quad .Lfunc_begin0 # function address
+; SKIP-BB-ENTRIES-NEXT: .byte 6 # number of basic blocks
+; SKIP-BB-ENTRIES-NEXT: .byte 100 # function entry count
+; SKIP-BB-ENTRIES-NOT: # BB id
diff --git a/llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test b/llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
new file mode 100644
index 00000000000000..13dc15932ede92
--- /dev/null
+++ b/llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
@@ -0,0 +1,96 @@
+## This test checks how llvm-readobj prints for skipped BB entries(-skip-emit-bb-entries) file.
+
+## Check 64-bit:
+# RUN: yaml2obj %s -DBITS=64 -o %t1.x64.o
+# RUN: llvm-readobj %t1.x64.o --bb-addr-map 2>&1 | FileCheck --match-full-lines %s -DFILE=%t1.x64.o
+
+## Check 32-bit:
+# RUN: yaml2obj %s -DBITS=32 -o %t1.x32.o
+# RUN: llvm-readobj %t1.x32.o --bb-addr-map 2>&1 | FileCheck --match-full-lines %s -DFILE=%t1.x32.o
+
+
+# CHECK: BBAddrMap [
+# CHECK-NEXT: Function {
+# CHECK-NEXT: At: 0x11111
+# CHECK-NEXT: Name: foo
+# CHECK-NEXT: BB Ranges [
+# CHECK-NEXT: {
+# CHECK-NEXT: Base Address: 0x11111
+# CHECK-NEXT: BB Entries [
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT: PGO analyses {
+# CHECK-NEXT: FuncEntryCount: 100
+# CHECK-NEXT: PGO BB entries [
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT:]
+# CHECK-NEXT:BBAddrMap [
+# CHECK-NEXT: Function {
+# CHECK-NEXT: At: 0x33333
+# CHECK-NEXT: Name: bar
+# CHECK-NEXT: BB Ranges [
+# CHECK-NEXT: {
+# CHECK-NEXT: Base Address: 0x33333
+# CHECK-NEXT: BB Entries [
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT: PGO analyses {
+# CHECK-NEXT: FuncEntryCount: 89
+# CHECK-NEXT: PGO BB entries [
+# CHECK-NEXT: ]
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT:]
+
+
+
+
+
+--- !ELF
+FileHeader:
+ Class: ELFCLASS[[BITS]]
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+Sections:
+ - Name: .text
+ Type: SHT_PROGBITS
+ Flags: [SHF_ALLOC]
+ - Name: .text.bar
+ Type: SHT_PROGBITS
+ Flags: [SHF_ALLOC]
+ - Name: .llvm_bb_addr_map
+ Type: SHT_LLVM_BB_ADDR_MAP
+ ShSize: [[SIZE=<none>]]
+ Link: .text
+ Entries:
+ - Version: 2
+ Feature: 0x17
+ BBRanges:
+ - BaseAddress: 0x11111
+ BBEntries:
+ PGOAnalyses:
+ - FuncEntryCount: 100
+ - Name: '.llvm_bb_addr_map (1)'
+ Type: SHT_LLVM_BB_ADDR_MAP
+ Link: .text.bar
+ Entries:
+ - Version: 2
+ Feature: 0x17
+ BBRanges:
+ - BaseAddress: 0x33333
+ BBEntries:
+ PGOAnalyses:
+ - FuncEntryCount: 89
+Symbols:
+ - Name: foo
+ Section: .text
+ Type: STT_FUNC
+ Value: 0x11111
+ - Name: bar
+ Section: .text.bar
+ Type: STT_FUNC
+ Value: 0x33333
diff --git a/llvm/test/tools/yaml2obj/ELF/bb-addr-map-pgo-analysis-map.yaml b/llvm/test/tools/yaml2obj/ELF/bb-addr-map-pgo-analysis-map.yaml
index 4dfaf60be3c0ed..a4cb572e6d9932 100644
--- a/llvm/test/tools/yaml2obj/ELF/bb-addr-map-pgo-analysis-map.yaml
+++ b/llvm/test/tools/yaml2obj/ELF/bb-addr-map-pgo-analysis-map.yaml
@@ -66,7 +66,7 @@ Sections:
## Check that yaml2obj generates a warning when we use unsupported feature.
# RUN: yaml2obj --docnum=2 %s 2>&1 | FileCheck %s --check-prefix=INVALID-FEATURE
-# INVALID-FEATURE: warning: invalid encoding for BBAddrMap::Features: 0xff
+# INVALID-FEATURE: warning: invalid encoding for BBAddrMap::Features: 0xf0
--- !ELF
FileHeader:
@@ -79,5 +79,4 @@ Sections:
Entries:
- Version: 2
## Specify unsupported feature
- Feature: 0xFF
-
+ Feature: 0xF0
diff --git a/llvm/unittests/Object/ELFObjectFileTest.cpp b/llvm/unittests/Object/ELFObjectFileTest.cpp
index c13dc0e3fab898..2a0921690914b4 100644
--- a/llvm/unittests/Object/ELFObjectFileTest.cpp
+++ b/llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -1148,11 +1148,11 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
BBAddrMap E1 = {
{{0x11111, {{1, 0x0, 0x1, {false, true, false, false, false}}}}}};
- PGOAnalysisMap P1 = {892, {}, {true, false, false, false}};
+ PGOAnalysisMap P1 = {892, {}, {true, false, false, false, false}};
BBAddrMap E2 = {
{{0x22222, {{2, 0x0, 0x2, {false, false, true, false, false}}}}}};
PGOAnalysisMap P2 = {
- {}, {{BlockFrequency(343), {}}}, {false, true, false, false}};
+ {}, {{BlockFrequency(343), {}}}, {false, true, false, false, false}};
BBAddrMap E3 = {{{0x33333,
{{0, 0x0, 0x3, {false, true, true, false, false}},
{1, 0x3, 0x3, {false, false, true, false, false}},
@@ -1163,7 +1163,7 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
{2, BranchProbability::getRaw(0xeeee'eeee)}}},
{{}, {{2, BranchProbability::getRaw(0xffff'ffff)}}},
{{}, {}}},
- {false, false, true, false}};
+ {false, false, true, false, false}};
BBAddrMap E4 = {{{0x44444,
{{0, 0x0, 0x4, {false, false, false, true, true}},
{1, 0x4, 0x4, {false, false, false, false, false}},
@@ -1180,10 +1180,10 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
{3, BranchProbability::getRaw(0xeeee'eeee)}}},
{BlockFrequency(18), {{3, BranchProbability::getRaw(0xffff'ffff)}}},
{BlockFrequency(1000), {}}},
- {true, true, true, false}};
+ {true, true, true, false, false}};
BBAddrMap E5 = {
{{0x55555, {{2, 0x0, 0x2, {false, false, true, false, false}}}}}};
- PGOAnalysisMap P5 = {{}, {}, {false, false, false, false}};
+ PGOAnalysisMap P5 = {{}, {}, {false, false, false, false, false}};
BBAddrMap E6 = {
{{0x66666,
{{0, 0x0, 0x6, {false, true, true, false, false}},
@@ -1195,7 +1195,7 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
{2, BranchProbability::getRaw(0xcccc'cccc)}}},
{{}, {{2, BranchProbability::getRaw(0x8888'8888)}}},
{{}, {}}},
- {false, false, true, true}};
+ {false, false, true, true, false}};
std::vector<BBAddrMap> Section0BBAddrMaps = {E4, E5, E6};
std::vector<BBAddrMap> Section1BBAddrMaps = {E3};
diff --git a/llvm/unittests/Object/ELFTypesTest.cpp b/llvm/unittests/Object/ELFTypesTest.cpp
index f04d45cf0983c7..13130dde80ef10 100644
--- a/llvm/unittests/Object/ELFTypesTest.cpp
+++ b/llvm/unittests/Object/ELFTypesTest.cpp
@@ -102,15 +102,15 @@ static_assert(
TEST(ELFTypesTest, BBAddrMapFeaturesEncodingTest) {
const std::array<BBAddrMap::Features, 9> Decoded = {
- {{false, false, false, false},
- {true, false, false, false},
- {false, true, false, false},
- {false, false, true, false},
- {false, false, false, true},
- {true, true, false, false},
- {false, true, true, false},
- {false, true, true, true},
- {true, true, true, true}}};
+ {{false, false, false, false, false},
+ {true, false, false, false, false},
+ {false, true, false, false, false},
+ {false, false, true, false, false},
+ {false, false, false, true, false},
+ {true, true, false, false, false},
+ {false, true, true, false, false},
+ {false, true, true, true, false},
+ {true, true, true, true, false}}};
const std::array<uint8_t, 9> Encoded = {
{0b0000, 0b0001, 0b0010, 0b0100, 0b1000, 0b0011, 0b0110, 0b1110, 0b1111}};
for (const auto &[Feat, EncodedVal] : llvm::zip(Decoded, Encoded))
@@ -125,9 +125,9 @@ TEST(ELFTypesTest, BBAddrMapFeaturesEncodingTest) {
TEST(ELFTypesTest, BBAddrMapFeaturesInvalidEncodingTest) {
const std::array<std::string, 2> Errors = {
- "invalid encoding for BBAddrMap::Features: 0x10",
- "invalid encoding for BBAddrMap::Features: 0xff"};
- const std::array<uint8_t, 2> Values = {{0b10000, 0b1111'1111}};
+ "invalid encoding for BBAddrMap::Features: 0x20",
+ "invalid encoding for BBAddrMap::Features: 0xf0"};
+ const std::array<uint8_t, 2> Values = {{0b10'0000, 0b1111'0000}};
for (const auto &[Val, Error] : llvm::zip(Values, Errors)) {
EXPECT_THAT_ERROR(BBAddrMap::Features::decode(Val).takeError(),
FailedWithMessage(Error));
|
jh7370
reviewed
Nov 1, 2024
Collaborator
jh7370
left a comment
jh7370
left a comment
There was a problem hiding this comment.
Drive-by comment since it touched some llvm-readobj code that pinged me. I'll leave the BB addr map people to review this properly.
llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
Outdated
Show resolved
Hide resolved
llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
Outdated
Show resolved
Hide resolved
Contributor
Author
|
No idea why the test only failed on windows, it seems just the flag doesn't take effect... I don't have windows sever, try to add some debug print into the code.. |
Contributor
Author
Fixed.. needs to use |
Contributor
Author
|
Ping:) @rlavaee , I really appreciate if you could review this patch, since most of the related code is from you. Basically, this is no-op to existing feature without enabling the switch. |
Contributor
|
Sorry for the late review. |
rlavaee
reviewed
Nov 12, 2024
rlavaee
reviewed
Nov 12, 2024
rlavaee
reviewed
Nov 12, 2024
Contributor
Author
|
Thank you for the code review! @rlavaee
I thought "BBAddrMap" should imply "HasBBEntries" by default, was to avoid the situation that "people want to use BBAddrMap, but have to explicitly code |
rlavaee
reviewed
Nov 13, 2024
llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
Outdated
Show resolved
Hide resolved
rlavaee
reviewed
Nov 13, 2024
rlavaee
reviewed
Nov 13, 2024
rlavaee
approved these changes
Nov 13, 2024
jh7370
reviewed
Nov 14, 2024
Collaborator
jh7370
left a comment
jh7370
left a comment
There was a problem hiding this comment.
Some nits in the new llvm-readobj test. Happy for this to go on once those are done (I've not reviewed the rest of the PR really).
llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
Outdated
Show resolved
Hide resolved
llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
Outdated
Show resolved
Hide resolved
llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
Outdated
Show resolved
Hide resolved
wlei-llvm
commented
Nov 14, 2024
Contributor
Author
wlei-llvm
left a comment
wlei-llvm
left a comment
There was a problem hiding this comment.
Some nits in the new llvm-readobj test. Happy for this to go on once those are done (I've not reviewed the rest of the PR really).
Thank you for the suggestions!
llvm/test/tools/llvm-readobj/ELF/bb-addr-map-skip-bb-entries.test
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes we want to use a
PgoAnalysisMapfeature that doesn't require the BB entries info, e.g. only theFuncEntryCount, but the BB entries is emitted by default, so I'm adding an option to skip the info for this case to save the binary size(can save ~90% size of the section). For implementation, it extends a new field(OmitBBEntries) inBBAddrMap::Featuresfor this and it's controlled by a switch--basic-block-address-map-skip-bb-entries.Note that this naturally supports backwards compatibility as the field is zero for the old version, matches the decoding in the new version llvm.