Skip to content

[BOLT] Use aggregated FuncBranchData in writeBATYAML #91289

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 13, 2024
Merged

[BOLT] Use aggregated FuncBranchData in writeBATYAML #91289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e4239c
[𝘀𝗽𝗿] changes to main this commit is based on
aaupov May 7, 2024
049c1e3
[𝘀𝗽𝗿] initial version
aaupov May 7, 2024
eecff96
[𝘀𝗽𝗿] changes introduced through rebase
aaupov May 11, 2024
b8425a4
Avoid the revert
aaupov May 11, 2024
4370534
[𝘀𝗽𝗿] changes introduced through rebase
aaupov May 13, 2024
2b8664c
rebase
aaupov May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 20 additions & 43 deletions bolt/lib/Profile/DataAggregator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2355,30 +2355,6 @@ std::error_code DataAggregator::writeBATYAML(BinaryContext &BC,
for (auto BI = BlockMap.begin(), BE = BlockMap.end(); BI != BE; ++BI)
YamlBF.Blocks[BI->second.getBBIndex()].Hash = BI->second.getBBHash();

auto getSuccessorInfo = [&](uint32_t SuccOffset, unsigned SuccDataIdx) {
const llvm::bolt::BranchInfo &BI = Branches.Data.at(SuccDataIdx);
yaml::bolt::SuccessorInfo SI;
SI.Index = BlockMap.getBBIndex(SuccOffset);
SI.Count = BI.Branches;
SI.Mispreds = BI.Mispreds;
return SI;
};

auto getCallSiteInfo = [&](Location CallToLoc, unsigned CallToIdx,
uint32_t Offset) {
const llvm::bolt::BranchInfo &BI = Branches.Data.at(CallToIdx);
yaml::bolt::CallSiteInfo CSI;
CSI.DestId = 0; // designated for unknown functions
CSI.EntryDiscriminator = 0;
CSI.Count = BI.Branches;
CSI.Mispreds = BI.Mispreds;
CSI.Offset = Offset;
if (BinaryData *BD = BC.getBinaryDataByName(CallToLoc.Name))
YAMLProfileWriter::setCSIDestination(BC, CSI, BD->getSymbol(), BAT,
CallToLoc.Offset);
return CSI;
};

// Lookup containing basic block offset and index
auto getBlock = [&BlockMap](uint32_t Offset) {
auto BlockIt = BlockMap.upper_bound(Offset);
Expand All @@ -2390,25 +2366,26 @@ std::error_code DataAggregator::writeBATYAML(BinaryContext &BC,
return std::pair(BlockIt->first, BlockIt->second.getBBIndex());
};

for (const auto &[FromOffset, SuccKV] : Branches.IntraIndex) {
const auto &[_, Index] = getBlock(FromOffset);
yaml::bolt::BinaryBasicBlockProfile &YamlBB = YamlBF.Blocks[Index];
for (const auto &[SuccOffset, SuccDataIdx] : SuccKV)
if (BlockMap.isInputBlock(SuccOffset))
YamlBB.Successors.emplace_back(
getSuccessorInfo(SuccOffset, SuccDataIdx));
}
for (const auto &[FromOffset, CallTo] : Branches.InterIndex) {
const auto &[BlockOffset, BlockIndex] = getBlock(FromOffset);
yaml::bolt::BinaryBasicBlockProfile &YamlBB = YamlBF.Blocks[BlockIndex];
const uint32_t Offset = FromOffset - BlockOffset;
for (const auto &[CallToLoc, CallToIdx] : CallTo)
YamlBB.CallSites.emplace_back(
getCallSiteInfo(CallToLoc, CallToIdx, Offset));
llvm::sort(YamlBB.CallSites, [](yaml::bolt::CallSiteInfo &A,
yaml::bolt::CallSiteInfo &B) {
return A.Offset < B.Offset;
});
for (const llvm::bolt::BranchInfo &BI : Branches.Data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need for llvm::bolt::.

Copy link
Contributor Author

@aaupov aaupov May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately we need a FQ name:
llvm::bolt::BranchInfo:

llvm-project/bolt/include/bolt/Profile/DataReader.h

Line 78 in 5944579

struct BranchInfo {

llvm::bolt::DataAggregator::BranchInfo:

llvm-project/bolt/include/bolt/Profile/DataAggregator.h

Line 125 in 5944579

struct BranchInfo {

I thought about renaming the latter to TakenInfo to match its counterpart FTInfo:

llvm-project/bolt/include/bolt/Profile/DataAggregator.h

Line 120 in 5944579

struct FTInfo {

WDYT?

Copy link
Contributor Author

@aaupov aaupov May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Dropped in a follow-up diff #92017.

using namespace yaml::bolt;
const auto &[BlockOffset, BlockIndex] = getBlock(BI.From.Offset);
BinaryBasicBlockProfile &YamlBB = YamlBF.Blocks[BlockIndex];
if (BI.To.IsSymbol && BI.To.Name == BI.From.Name && BI.To.Offset != 0) {
// Internal branch
const unsigned SuccIndex = getBlock(BI.To.Offset).second;
auto &SI = YamlBB.Successors.emplace_back(SuccessorInfo{SuccIndex});
SI.Count = BI.Branches;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't this be part of constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can, but I decided to keep it this way for two reasons:

  1. type impedance (int vs uint) between BI and SI requiring a cast
  2. making it explicit where Branches and Mispreds are assigned (vs implicitly in constructor parameter order).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type impedance means that if Count and Mispreds are passed to the constructor there needs to be an explicit cast (as the constructor doesn't accept uints). Whereas if the values are assigned to, there's an implicit cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, an explicit cast in the constructor due to type impedance:

llvm-project/bolt/lib/Profile/DataAggregator.cpp

Lines 1296 to 1298 in 31a203f

return AggregatedLBREntry{From.get(), To.get(),
static_cast<uint64_t>(Frequency.get()), Mispreds,
Type};

SI.Mispreds = BI.Mispreds;
} else {
// Call
const uint32_t Offset = BI.From.Offset - BlockOffset;
auto &CSI = YamlBB.CallSites.emplace_back(CallSiteInfo{Offset});
CSI.Count = BI.Branches;
CSI.Mispreds = BI.Mispreds;
if (const BinaryData *BD = BC.getBinaryDataByName(BI.To.Name))
YAMLProfileWriter::setCSIDestination(BC, CSI, BD->getSymbol(), BAT,
BI.To.Offset);
}
}
// Set entry counts, similar to DataReader::readProfile.
for (const llvm::bolt::BranchInfo &BI : Branches.EntryData) {
Expand Down
2 changes: 2 additions & 0 deletions bolt/test/X86/Inputs/blarge_new_bat_order.preagg.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
B 800154 401050 20 0
F 800159 800193 7
11 changes: 11 additions & 0 deletions bolt/test/X86/bolt-address-translation-yaml.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ BRANCHENTRY-YAML-CHECK: - name: SolveCubic
BRANCHENTRY-YAML-CHECK: bid: 0
BRANCHENTRY-YAML-CHECK: hash: 0x700F19D24600000
BRANCHENTRY-YAML-CHECK-NEXT: succ: [ { bid: 7, cnt: 1 }
# Check that the order is correct between BAT YAML and FDATA->YAML.
RUN: perf2bolt %t.out --pa -p %p/Inputs/blarge_new_bat_order.preagg.txt \
RUN: -w %t.yaml -o %t.fdata
RUN: llvm-bolt %t.exe -data %t.fdata -w %t.yaml-fdata -o %t.null
RUN: FileCheck --input-file %t.yaml --check-prefix ORDER-YAML-CHECK %s
RUN: FileCheck --input-file %t.yaml-fdata --check-prefix ORDER-YAML-CHECK %s
ORDER-YAML-CHECK: - name: SolveCubic
ORDER-YAML-CHECK: bid: 3
ORDER-YAML-CHECK: hash: 0xDDA1DC5F69F900AC
ORDER-YAML-CHECK-NEXT: calls: [ { off: 0x26, fid: [[#]], cnt: 20 } ]
ORDER-YAML-CHECK-NEXT: succ: [ { bid: 5, cnt: 7 }
# Large profile test
RUN: perf2bolt %t.out --pa -p %p/Inputs/blarge_new_bat.preagg.txt -w %t.yaml -o %t.fdata \
RUN: 2>&1 | FileCheck --check-prefix READ-BAT-CHECK %s
Expand Down
Loading