[llvm-objdump] Add support for symbolizing PGOBBAddrMap Info #76386
Conversation
boomanaiden154
requested review from
MaskRay,
rlavaee,
jh7370 and
red1bluelost
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Aiden Grossman (boomanaiden154) ChangesThis patch adds in support for symbolizing PGO information contained within the SHT_LLVM_BB_ADDR_MAP section in llvm-objdump. The outputs are simply the raw values contained within the section. Full diff: https://github.com/llvm/llvm-project/pull/76386.diff 2 Files Affected:
diff --git a/llvm/test/tools/llvm-objdump/X86/elf-pgobbaddrmap.yaml b/llvm/test/tools/llvm-objdump/X86/elf-pgobbaddrmap.yaml
new file mode 100644
index 00000000000000..529a64da2ddf57
--- /dev/null
+++ b/llvm/test/tools/llvm-objdump/X86/elf-pgobbaddrmap.yaml
@@ -0,0 +1,181 @@
+# Test that in the presence of SHT_LLVM_BB_ADDR_MAP sections which also
+# contain PGO data, --symbolize-operands is able to label the basic blocks
+# correctly.
+
+# Check the case where we only have entry counts.
+
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-objdump %t1 -d --symbolize-operands --no-show-raw-insn --no-leading-addr | \
+# RUN: FileCheck %s --check-prefix=ENTRYCOUNT
+
+--- !ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
+ - Name: .text.foo
+ Type: SHT_PROGBITS
+ Address: 0x0
+ Flags: [SHF_ALLOC, SHF_EXECINSTR]
+ Content: '50'
+ - Name: .llvm_bb_addr_map.foo
+ Type: SHT_LLVM_BB_ADDR_MAP
+ Link: .text.foo
+ Entries:
+ - Version: 2
+ Address: 0x0
+ Feature: 0x1
+ BBEntries:
+ - ID: 3
+ AddressOffset: 0x0
+ Size: 0x1
+ Metadata: 0x1
+ PGOAnalyses:
+ - FuncEntryCount: 1000
+Symbols:
+ - Name: foo
+ Section: .text.foo
+ Value: 0x0
+
+# ENTRYCOUNT: <foo>:
+# ENTRYCOUNT: <BB3> (Entry count: 1000):
+
+# Check the case where we have entrypoints and block frequency information
+
+# RUN: yaml2obj %s --docnum=2 -o %t2
+# RUN: llvm-objdump %t2 -d --symbolize-operands --no-show-raw-insn --no-leading-addr | \
+# RUN: FileCheck %s --check-prefix=ENTRYCOUNT-BLOCKFREQ
+
+--- !ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
+ - Name: .text.foo
+ Type: SHT_PROGBITS
+ Address: 0x0
+ Flags: [SHF_ALLOC, SHF_EXECINSTR]
+ Content: '503b0505200000907d02ebf5c3'
+ - Name: .llvm_bb_addr_map.foo
+ Type: SHT_LLVM_BB_ADDR_MAP
+ Link: .text.foo
+ Entries:
+ - Version: 2
+ Address: 0x0
+ Feature: 0x3
+ BBEntries:
+ - ID: 3
+ AddressOffset: 0x0
+ Size: 0x1
+ Metadata: 0x1
+ - ID: 1
+ AddressOffset: 0x0
+ Size: 0x6
+ Metadata: 0x0
+ - ID: 2
+ AddressOffset: 0x1
+ Size: 0x4
+ Metadata: 0x0
+ - ID: 5
+ AddressOffset: 0x0
+ Size: 0x1
+ Metadata: 0x2
+ PGOAnalyses:
+ - FuncEntryCount: 1000
+ PGOBBEntries:
+ - BBFreq: 1000
+ - BBFreq: 133
+ - BBFreq: 18
+ - BBFreq: 1000
+Symbols:
+ - Name: foo
+ Section: .text.foo
+ Value: 0x0
+
+# ENTRYCOUNT-BLOCKFREQ: <foo>:
+# ENTRYCOUNT-BLOCKFREQ: <BB3> (Entry count: 1000, Frequency: 1000):
+# ENTRYCOUNT-BLOCKFREQ: <BB1> (Frequency: 133):
+# ENTRYCOUNT-BLOCKFREQ: <BB2> (Frequency: 18):
+# ENTRYCOUNT-BLOCKFREQ: <BB5> (Frequency: 1000):
+
+# Check the case where we have entrypoints, block frequency, and branch
+# proabability information.
+
+# RUN: yaml2obj %s --docnum=3 -o %t3
+# RUN: llvm-objdump %t3 -d --symbolize-operands --no-show-raw-insn --no-leading-addr | \
+# RUN: FileCheck %s --check-prefix=ENTRY-FREQ-PROB
+
+--- !ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
+ - Name: .text.foo
+ Type: SHT_PROGBITS
+ Address: 0x0
+ Flags: [SHF_ALLOC, SHF_EXECINSTR]
+ Content: '503b0505200000907d02ebf5c3'
+ - Name: .llvm_bb_addr_map.foo
+ Type: SHT_LLVM_BB_ADDR_MAP
+ Link: .text.foo
+ Entries:
+ - Version: 2
+ Address: 0x0
+ Feature: 0x7
+ BBEntries:
+ - ID: 3
+ AddressOffset: 0x0
+ Size: 0x1
+ Metadata: 0x1
+ - ID: 1
+ AddressOffset: 0x0
+ Size: 0x6
+ Metadata: 0x0
+ - ID: 2
+ AddressOffset: 0x1
+ Size: 0x4
+ Metadata: 0x0
+ - ID: 5
+ AddressOffset: 0x0
+ Size: 0x1
+ Metadata: 0x2
+ PGOAnalyses:
+ - FuncEntryCount: 1000
+ PGOBBEntries:
+ - BBFreq: 1000
+ Successors:
+ - ID: 1
+ BrProb: 0x22222222
+ - ID: 2
+ BrProb: 0x33333333
+ - ID: 3
+ BrProb: 0xaaaaaaaa
+ - BBFreq: 133
+ Successors:
+ - ID: 2
+ BrProb: 0x11111111
+ - ID: 3
+ BrProb: 0xeeeeeeee
+ - BBFreq: 18
+ Successors:
+ - ID: 3
+ BrProb: 0xffffffff
+ - BBFreq: 1000
+ Successors: []
+Symbols:
+ - Name: foo
+ Section: .text.foo
+ Value: 0x0
+
+# ENTRY-FREQ-PROB: <foo>:
+# ENTRY-FREQ-PROB: <BB3> (Entry count: 1000, Frequency: 1000, Successors: BB1-572662306, BB2-858993459, BB3-2863311530):
+# ENTRY-FREQ-PROB: <BB1> (Frequency: 133, Successors: BB2-286331153, BB3-4008636142):
+# ENTRY-FREQ-PROB: <BB2> (Frequency: 18, Successors: BB3-4294967295):
+# ENTRY-FREQ-PROB: <BB5> (Frequency: 1000):
+
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 7467a6062b5a8b..9728ec61969c06 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -1264,23 +1264,70 @@ static SymbolInfoTy createDummySymbolInfo(const ObjectFile &Obj,
return SymbolInfoTy(Addr, Name, Type);
}
-static void
-collectBBAddrMapLabels(const std::unordered_map<uint64_t, BBAddrMap> &AddrToBBAddrMap,
- uint64_t SectionAddr, uint64_t Start, uint64_t End,
- std::unordered_map<uint64_t, std::vector<std::string>> &Labels) {
+static void collectBBAddrMapLabels(
+ const std::unordered_map<uint64_t, BBAddrMap> &AddrToBBAddrMap,
+ const std::unordered_map<uint64_t, PGOAnalysisMap> &AddrToPGOBBAddrMap,
+ uint64_t SectionAddr, uint64_t Start, uint64_t End,
+ std::unordered_map<
+ uint64_t, std::vector<std::pair<std::string, std::string>>> &Labels) {
if (AddrToBBAddrMap.empty())
return;
Labels.clear();
uint64_t StartAddress = SectionAddr + Start;
uint64_t EndAddress = SectionAddr + End;
auto Iter = AddrToBBAddrMap.find(StartAddress);
+ auto PGOIter = AddrToPGOBBAddrMap.find(StartAddress);
if (Iter == AddrToBBAddrMap.end())
return;
- for (const BBAddrMap::BBEntry &BBEntry : Iter->second.getBBEntries()) {
+ for (size_t I = 0; I < Iter->second.getBBEntries().size(); ++I) {
+ const BBAddrMap::BBEntry &BBEntry = Iter->second.getBBEntries()[I];
uint64_t BBAddress = BBEntry.Offset + Iter->second.getFunctionAddress();
if (BBAddress >= EndAddress)
continue;
- Labels[BBAddress].push_back(("BB" + Twine(BBEntry.ID)).str());
+
+ std::string LabelString = "BB" + Twine(BBEntry.ID).str();
+ std::string PGOString = "";
+
+ if (!AddrToPGOBBAddrMap.empty()) {
+ assert(PGOIter != AddrToPGOBBAddrMap.end() &&
+ "PGOAnalysisMap and BBAddrMap should have information on the same "
+ "basic blocks");
+ const PGOAnalysisMap::PGOBBEntry &PGOBBEntry =
+ PGOIter->second.BBEntries[I];
+
+ PGOString += " (";
+
+ if (PGOIter->second.FeatEnable.FuncEntryCount && I == 0) {
+ PGOString +=
+ "Entry count: " + Twine(PGOIter->second.FuncEntryCount).str();
+ if (PGOIter->second.FeatEnable.BBFreq ||
+ PGOIter->second.FeatEnable.BrProb) {
+ PGOString += ", ";
+ }
+ }
+ if (PGOIter->second.FeatEnable.BBFreq) {
+ PGOString +=
+ "Frequency: " + Twine(PGOBBEntry.BlockFreq.getFrequency()).str();
+ if (PGOIter->second.FeatEnable.BrProb &&
+ PGOBBEntry.Successors.size() > 0) {
+ PGOString += ", ";
+ }
+ }
+ if (PGOIter->second.FeatEnable.BrProb &&
+ PGOBBEntry.Successors.size() > 0) {
+ PGOString += "Successors: ";
+ for (size_t J = 0; J < PGOBBEntry.Successors.size(); ++J) {
+ if (J != 0)
+ PGOString += ", ";
+ const auto &PGOSuccessorEntry = PGOBBEntry.Successors[J];
+ PGOString += "BB" + Twine(PGOSuccessorEntry.ID).str() + "-" +
+ Twine(PGOSuccessorEntry.Prob.getNumerator()).str();
+ }
+ }
+ PGOString += ")";
+ }
+
+ Labels[BBAddress].push_back(std::make_pair(LabelString, PGOString));
}
}
@@ -1638,11 +1685,13 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
LLVM_DEBUG(LVP.dump());
std::unordered_map<uint64_t, BBAddrMap> AddrToBBAddrMap;
+ std::unordered_map<uint64_t, PGOAnalysisMap> AddrToPGOBBAddrMap;
auto ReadBBAddrMap = [&](std::optional<unsigned> SectionIndex =
std::nullopt) {
AddrToBBAddrMap.clear();
if (const auto *Elf = dyn_cast<ELFObjectFileBase>(&Obj)) {
- auto BBAddrMapsOrErr = Elf->readBBAddrMap(SectionIndex);
+ std::vector<PGOAnalysisMap> PGOAnalyses;
+ auto BBAddrMapsOrErr = Elf->readBBAddrMap(SectionIndex, &PGOAnalyses);
if (!BBAddrMapsOrErr) {
reportWarning(toString(BBAddrMapsOrErr.takeError()), Obj.getFileName());
return;
@@ -1650,6 +1699,13 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
for (auto &FunctionBBAddrMap : *BBAddrMapsOrErr)
AddrToBBAddrMap.emplace(FunctionBBAddrMap.Addr,
std::move(FunctionBBAddrMap));
+ for (size_t I = 0; I < (*BBAddrMapsOrErr).size(); ++I) {
+ AddrToBBAddrMap.emplace((*BBAddrMapsOrErr)[I].Addr,
+ std::move((*BBAddrMapsOrErr)[I]));
+ if (PGOAnalyses.size() > 0)
+ AddrToPGOBBAddrMap.emplace((*BBAddrMapsOrErr)[I].Addr,
+ std::move(PGOAnalyses[I]));
+ }
}
};
@@ -1978,14 +2034,16 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
FOS.SetUnbuffered();
std::unordered_map<uint64_t, std::string> AllLabels;
- std::unordered_map<uint64_t, std::vector<std::string>> BBAddrMapLabels;
+ std::unordered_map<uint64_t,
+ std::vector<std::pair<std::string, std::string>>>
+ BBAddrMapLabels;
if (SymbolizeOperands) {
collectLocalBranchTargets(Bytes, DT->InstrAnalysis.get(),
DT->DisAsm.get(), DT->InstPrinter.get(),
PrimaryTarget.SubtargetInfo.get(),
SectionAddr, Index, End, AllLabels);
- collectBBAddrMapLabels(AddrToBBAddrMap, SectionAddr, Index, End,
- BBAddrMapLabels);
+ collectBBAddrMapLabels(AddrToBBAddrMap, AddrToPGOBBAddrMap, SectionAddr,
+ Index, End, BBAddrMapLabels);
}
if (DT->InstrAnalysis)
@@ -2083,8 +2141,9 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
// Print local label if there's any.
auto Iter1 = BBAddrMapLabels.find(SectionAddr + Index);
if (Iter1 != BBAddrMapLabels.end()) {
- for (StringRef Label : Iter1->second)
- FOS << "<" << Label << ">:\n";
+ for (const auto &LabelParts : Iter1->second)
+ FOS << "<" << std::get<0>(LabelParts) << ">"
+ << std::get<1>(LabelParts) << ":\n";
} else {
auto Iter2 = AllLabels.find(SectionAddr + Index);
if (Iter2 != AllLabels.end())
@@ -2261,7 +2320,7 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
} else if (!Disp) {
*TargetOS << TargetName;
} else if (BBAddrMapLabelAvailable) {
- *TargetOS << BBAddrMapLabels[Target].front();
+ *TargetOS << std::get<0>(BBAddrMapLabels[Target].front());
} else if (LabelAvailable) {
*TargetOS << AllLabels[Target];
} else {
@@ -2277,7 +2336,9 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
}
} else if (BBAddrMapLabelAvailable) {
- *TargetOS << " <" << BBAddrMapLabels[Target].front() << ">";
+ *TargetOS << " <"
+ << std::get<0>(BBAddrMapLabels[Target].front())
+ << ">";
} else if (LabelAvailable) {
*TargetOS << " <" << AllLabels[Target] << ">";
}
|
This patch adds in support for symbolizing PGO information contained within the SHT_LLVM_BB_ADDR_MAP section in llvm-objdump. The outputs are simply the raw values contained within the section.
boomanaiden154
force-pushed
the
llvm-objdump-symbolize-pgo-bb-addr-map
branch
from
203c71f
to
0878d07
Compare
|
@rlavaee Any further comments on this patch? |
| for (auto &FunctionBBAddrMap : *BBAddrMapsOrErr) | ||
| AddrToBBAddrMap.emplace(FunctionBBAddrMap.Addr, | ||
| std::move(FunctionBBAddrMap)); | ||
| for (auto &&[FunctionBBAddrMap, FunctionPGOAnalysis] : |
There was a problem hiding this comment.
Why do we use auto && here? We need non-const references. So auto & should be sufficient. right?
There was a problem hiding this comment.
It was suggested above by @red1bluelost. Not entirely sure what the reasoning was. Using a lvalue reference does create a temporary object here, but I don't think that should matter? The temporary object does prevent the creation of a non-const lvalue reference though, and I wouldn't really consider this to be a const use of the values from the iterator (although it does compile).
There was a problem hiding this comment.
auto & and auto && should not make a difference in the generated code for this case. The zip_equal might be a temporary object but the AddrMap and AnalysisMap iterators will be giving us auto & regardless of which one we picked in the ranged-for loop. Part of my reasoning (beyond it just being the habit I've built) for using auto && is the fact that we move values out within the loop.
There was a problem hiding this comment.
Makes sense. Given auto & won't bind to the reference without a const, I think keeping it as auto && makes sense given we're moving the values. @rlavaee what do you think?
There was a problem hiding this comment.
Thanks. I see what's happening now. Range over zip_equal returns a temporary tuple. So you can't take a non-const reference. auto && is the right choice here. You can also use zip_equal(*std::move(BBAddrMapsOrErr), std::move(PGOAnalyses)) to avoid unnecessary copies.
There was a problem hiding this comment.
Good point. Fixed in the latest iteration.
Thank you for the review! Sorry about that. I'll make sure to leave them open next time. |
|
the Look here for details https://lab.llvm.org/buildbot/#/builders/233/builds/6205 |
|
Thanks for catching that. I pushed a short patch (99ffe71) to disable that test on windows since we also do not support bb-addr-map on windows. Relevant lines where we disable the bb-addr-map test: |
Could you clarify what is platform-specific behaviour that means this doesn't work on Windows, please? On the surface of it, the code in this patch should just work on Windows as far as I can see. |
The pgo-analysis-map is effectively a super-set of anything bb-addr-map does, so they share this issue. From digging into the chain in this issue (#60013), there is problems between VS2019 and VS2022. The tests run with VS2022 which fail on the bb-addr-map feature. I tried to build on windows to test it out but I couldn't get lit working well enough to debug the issue. From digging in the issues chain, others had difficulty with debug Windows and didn't have the time to dedicate to solving it. |
|
I managed to get it working on Windows and have been running WinDBG. Seems like when its decoding the BBAddrMap, it is getting incorrect values for when reading ULEB128 values. I'll try to look into it more if I have time. |
Thanks. It's probably worth filing an issue to note this (if there isn't one already), since it sounds like a bug that there's just not the motivation to fix (because the only users are Linux-based at the moment). That way, even if you don't make progress, we've at least acknowledged the issue and have somewhere we can point to in tests that have been disabled etc. |
This patch adds in support for symbolizing PGO information contained within the SHT_LLVM_BB_ADDR_MAP section in llvm-objdump. The outputs are simply the raw values contained within the section.