[llvm] [BOLT] Ignore returns in DataAggregator (PR #90807)
Amir Ayupov via llvm-commits
llvm-commits at lists.llvm.org
Wed May 8 12:02:18 PDT 2024
https://github.com/aaupov updated https://github.com/llvm/llvm-project/pull/90807
>From 2b1be3d4ca776d4fbcf6753078eec0ed268ff552 Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Wed, 1 May 2024 18:09:38 -0700
Subject: [PATCH 1/6] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?=
=?UTF-8?q?anges=20to=20main=20this=20commit=20is=20based=20on?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Created using spr 1.3.4
[skip ci]
---
bolt/include/bolt/Core/BinaryFunction.h | 6 ++++++
bolt/lib/Core/BinaryFunction.cpp | 6 +-----
bolt/lib/Passes/ValidateInternalCalls.cpp | 13 ++++---------
bolt/lib/Rewrite/RewriteInstance.cpp | 3 ++-
4 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index 26d2d01f86267..540a9767760ad 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -361,6 +361,9 @@ class BinaryFunction {
/// True if another function body was merged into this one.
bool HasFunctionsFoldedInto{false};
+ /// True if the function has internal calls.
+ bool HasInternalCalls{false};
+
/// Name for the section this function code should reside in.
std::string CodeSectionName;
@@ -1334,6 +1337,9 @@ class BinaryFunction {
/// Return true if other functions were folded into this one.
bool hasFunctionsFoldedInto() const { return HasFunctionsFoldedInto; }
+ /// Return true if the function has internal calls.
+ bool hasInternalCalls() const { return HasInternalCalls; }
+
/// If this function was folded, return the function it was folded into.
BinaryFunction *getFoldedIntoFunction() const { return FoldedIntoFunction; }
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 1fa96dfaabde8..8b12492aa1a13 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1281,6 +1281,7 @@ Error BinaryFunction::disassemble() {
// Recursive call.
TargetSymbol = getSymbol();
} else {
+ HasInternalCalls = true;
if (BC.isX86()) {
// Dangerous old-style x86 PIC code. We may need to freeze this
// function, so preserve the function as is for now.
@@ -1436,11 +1437,6 @@ Error BinaryFunction::disassemble() {
clearList(Relocations);
- if (!IsSimple) {
- clearList(Instructions);
- return createNonFatalBOLTError("");
- }
-
updateState(State::Disassembled);
return Error::success();
diff --git a/bolt/lib/Passes/ValidateInternalCalls.cpp b/bolt/lib/Passes/ValidateInternalCalls.cpp
index 88df2e5b59f38..24f9bfde401ae 100644
--- a/bolt/lib/Passes/ValidateInternalCalls.cpp
+++ b/bolt/lib/Passes/ValidateInternalCalls.cpp
@@ -309,15 +309,10 @@ Error ValidateInternalCalls::runOnFunctions(BinaryContext &BC) {
std::set<BinaryFunction *> NeedsValidation;
for (auto &BFI : BC.getBinaryFunctions()) {
BinaryFunction &Function = BFI.second;
- for (BinaryBasicBlock &BB : Function) {
- for (MCInst &Inst : BB) {
- if (getInternalCallTarget(Function, Inst)) {
- NeedsValidation.insert(&Function);
- Function.setSimple(false);
- break;
- }
- }
- }
+ if (!Function.hasInternalCalls())
+ continue;
+ NeedsValidation.insert(&Function);
+ Function.setSimple(false);
}
// Skip validation for non-relocation mode
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 23f79e3c135a7..f6f597c8cee0d 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -3343,7 +3343,8 @@ void RewriteInstance::disassembleFunctions() {
if (!Function.isSimple()) {
assert((!BC->HasRelocations || Function.getSize() == 0 ||
- Function.hasIndirectTargetToSplitFragment()) &&
+ Function.hasIndirectTargetToSplitFragment() ||
+ Function.hasInternalCalls()) &&
"unexpected non-simple function in relocation mode");
continue;
}
>From f21c7c5579eaf8bfd481e2c87920b8e95f9c8ecc Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Thu, 2 May 2024 16:26:05 -0700
Subject: [PATCH 2/6] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?=
=?UTF-8?q?anges=20introduced=20through=20rebase?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Created using spr 1.3.4
[skip ci]
---
bolt/include/bolt/Core/BinaryFunction.h | 6 ------
bolt/lib/Core/BinaryFunction.cpp | 6 +++++-
bolt/lib/Passes/ValidateInternalCalls.cpp | 13 +++++++++----
bolt/lib/Rewrite/RewriteInstance.cpp | 3 +--
4 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index 540a9767760ad..26d2d01f86267 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -361,9 +361,6 @@ class BinaryFunction {
/// True if another function body was merged into this one.
bool HasFunctionsFoldedInto{false};
- /// True if the function has internal calls.
- bool HasInternalCalls{false};
-
/// Name for the section this function code should reside in.
std::string CodeSectionName;
@@ -1337,9 +1334,6 @@ class BinaryFunction {
/// Return true if other functions were folded into this one.
bool hasFunctionsFoldedInto() const { return HasFunctionsFoldedInto; }
- /// Return true if the function has internal calls.
- bool hasInternalCalls() const { return HasInternalCalls; }
-
/// If this function was folded, return the function it was folded into.
BinaryFunction *getFoldedIntoFunction() const { return FoldedIntoFunction; }
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 8b12492aa1a13..1fa96dfaabde8 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1281,7 +1281,6 @@ Error BinaryFunction::disassemble() {
// Recursive call.
TargetSymbol = getSymbol();
} else {
- HasInternalCalls = true;
if (BC.isX86()) {
// Dangerous old-style x86 PIC code. We may need to freeze this
// function, so preserve the function as is for now.
@@ -1437,6 +1436,11 @@ Error BinaryFunction::disassemble() {
clearList(Relocations);
+ if (!IsSimple) {
+ clearList(Instructions);
+ return createNonFatalBOLTError("");
+ }
+
updateState(State::Disassembled);
return Error::success();
diff --git a/bolt/lib/Passes/ValidateInternalCalls.cpp b/bolt/lib/Passes/ValidateInternalCalls.cpp
index 24f9bfde401ae..88df2e5b59f38 100644
--- a/bolt/lib/Passes/ValidateInternalCalls.cpp
+++ b/bolt/lib/Passes/ValidateInternalCalls.cpp
@@ -309,10 +309,15 @@ Error ValidateInternalCalls::runOnFunctions(BinaryContext &BC) {
std::set<BinaryFunction *> NeedsValidation;
for (auto &BFI : BC.getBinaryFunctions()) {
BinaryFunction &Function = BFI.second;
- if (!Function.hasInternalCalls())
- continue;
- NeedsValidation.insert(&Function);
- Function.setSimple(false);
+ for (BinaryBasicBlock &BB : Function) {
+ for (MCInst &Inst : BB) {
+ if (getInternalCallTarget(Function, Inst)) {
+ NeedsValidation.insert(&Function);
+ Function.setSimple(false);
+ break;
+ }
+ }
+ }
}
// Skip validation for non-relocation mode
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index f6f597c8cee0d..23f79e3c135a7 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -3343,8 +3343,7 @@ void RewriteInstance::disassembleFunctions() {
if (!Function.isSimple()) {
assert((!BC->HasRelocations || Function.getSize() == 0 ||
- Function.hasIndirectTargetToSplitFragment() ||
- Function.hasInternalCalls()) &&
+ Function.hasIndirectTargetToSplitFragment()) &&
"unexpected non-simple function in relocation mode");
continue;
}
>From acf58ceb37d2aa917e8d84d243faadc58f5f3a7d Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Mon, 6 May 2024 13:35:04 -0700
Subject: [PATCH 3/6] Simplify IsReturn check
Created using spr 1.3.4
---
bolt/lib/Profile/DataAggregator.cpp | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/bolt/lib/Profile/DataAggregator.cpp b/bolt/lib/Profile/DataAggregator.cpp
index e4a7324c38175..d02e4499014ed 100644
--- a/bolt/lib/Profile/DataAggregator.cpp
+++ b/bolt/lib/Profile/DataAggregator.cpp
@@ -778,13 +778,13 @@ bool DataAggregator::doBranch(uint64_t From, uint64_t To, uint64_t Count,
if (BinaryFunction *Func = getBinaryFunctionContainingAddress(Addr)) {
Addr -= Func->getAddress();
if (IsFrom) {
- if (Func->hasInstructions()) {
- if (MCInst *Inst = Func->getInstructionAtOffset(Addr))
- IsReturn = BC->MIB->isReturn(*Inst);
- } else if (std::optional<MCInst> Inst =
- Func->disassembleInstructionAtOffset(Addr)) {
- IsReturn = BC->MIB->isReturn(*Inst);
- }
+ auto checkReturn = [&](auto MaybeInst) {
+ IsReturn = MaybeInst && BC->MIB->isReturn(*MaybeInst);
+ };
+ if (Func->hasInstructions())
+ checkReturn(Func->getInstructionAtOffset(Addr));
+ else
+ checkReturn(Func->disassembleInstructionAtOffset(Addr));
}
if (BAT)
>From 22052e461e5671f376fe2dcb733446b0a63e956d Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Tue, 7 May 2024 18:30:48 -0700
Subject: [PATCH 4/6] drop const from disassembleInstructionAtOffset
Created using spr 1.3.4
---
bolt/include/bolt/Core/BinaryFunction.h | 3 +--
bolt/lib/Core/BinaryFunction.cpp | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index b21312f92c485..3c641581e247a 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -930,8 +930,7 @@ class BinaryFunction {
return const_cast<BinaryFunction *>(this)->getInstructionAtOffset(Offset);
}
- const std::optional<MCInst>
- disassembleInstructionAtOffset(uint64_t Offset) const;
+ std::optional<MCInst> disassembleInstructionAtOffset(uint64_t Offset) const;
/// Return offset for the first instruction. If there is data at the
/// beginning of a function then offset of the first instruction could
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 5f3c0cb1ad754..fb81fc3f2ba7b 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1167,7 +1167,7 @@ void BinaryFunction::handleAArch64IndirectCall(MCInst &Instruction,
}
}
-const std::optional<MCInst>
+std::optional<MCInst>
BinaryFunction::disassembleInstructionAtOffset(uint64_t Offset) const {
assert(CurrentState == State::Empty);
assert(Offset < MaxSize && "invalid offset");
>From 63725510bf85c9e3862800830f5881099ab4b21f Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Wed, 8 May 2024 11:59:59 -0700
Subject: [PATCH 5/6] Assert messages
Created using spr 1.3.4
---
bolt/lib/Core/BinaryFunction.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index fb81fc3f2ba7b..4721a247ee2e2 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1169,10 +1169,10 @@ void BinaryFunction::handleAArch64IndirectCall(MCInst &Instruction,
std::optional<MCInst>
BinaryFunction::disassembleInstructionAtOffset(uint64_t Offset) const {
- assert(CurrentState == State::Empty);
- assert(Offset < MaxSize && "invalid offset");
+ assert(CurrentState == State::Empty && "Function should not be disassembled");
+ assert(Offset < MaxSize && "Invalid offset");
ErrorOr<ArrayRef<unsigned char>> FunctionData = getData();
- assert(FunctionData && "cannot get function as data");
+ assert(FunctionData && "Cannot get function as data");
MCInst Instr;
uint64_t InstrSize = 0;
const uint64_t InstrAddress = getAddress() + Offset;
>From 6ec0d9b1838bb5ba5c1fbe5730736c057f186493 Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Wed, 8 May 2024 12:02:06 -0700
Subject: [PATCH 6/6] clang-format
Created using spr 1.3.4
---
bolt/lib/Core/BinaryFunction.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 4721a247ee2e2..de34421ebeb08 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1177,7 +1177,7 @@ BinaryFunction::disassembleInstructionAtOffset(uint64_t Offset) const {
uint64_t InstrSize = 0;
const uint64_t InstrAddress = getAddress() + Offset;
if (BC.DisAsm->getInstruction(Instr, InstrSize, FunctionData->slice(Offset),
- InstrAddress, nulls()))
+ InstrAddress, nulls()))
return Instr;
return std::nullopt;
}
More information about the llvm-commits
mailing list