[BOLT][BTI] Refactor BTI helpers #173000
Open
[BOLT][BTI] Refactor BTI helpers #173000
+75
−77
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
- Add an enum to encode BTI variants in function arguments. - Removed updateBTIVariant as createBTI can be used for the same purpose. - Removed a test case that checked against invalid BTI variants, as those are unrepresentable.
bgergely0
requested review from
aaupov,
ayermolo,
maksfb,
paschalis-mpeis,
rafaelauler,
yota9 and
yozhu
as code owners
Member
|
@llvm/pr-subscribers-bolt Author: Gergely Bálint (bgergely0) Changes
Full diff: https://github.com/llvm/llvm-project/pull/173000.diff 3 Files Affected:
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index 6d0ba466347c1..31bf73c839c3a 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -66,6 +66,14 @@ enum class IndirectBranchType : char {
/// PIC jump table.
};
+/// Enum used for readability when describing different BTI instruction
+/// variants. The variant is encoded as an immediate of the instruction in LLVM.
+enum BTIKind {
+ C, /// Accepting calls, and jumps using x16/x17.
+ J, /// Accepting jumps.
+ JC /// Accepting both.
+};
+
class MCPlusBuilder {
public:
using AllocatorIdTy = uint16_t;
@@ -1871,8 +1879,7 @@ class MCPlusBuilder {
/// Check if an Instruction is a BTI landing pad with the required properties.
/// Takes both explicit and implicit BTIs into account.
- virtual bool isBTILandingPad(MCInst &Inst, bool CallTarget,
- bool JumpTarget) const {
+ virtual bool isBTILandingPad(MCInst &Inst, BTIKind BTI) const {
llvm_unreachable("not implemented");
return false;
}
@@ -1884,13 +1891,7 @@ class MCPlusBuilder {
}
/// Create a BTI landing pad instruction.
- virtual void createBTI(MCInst &Inst, bool CallTarget, bool JumpTarget) const {
- llvm_unreachable("not implemented");
- }
-
- /// Update operand of BTI instruction.
- virtual void updateBTIVariant(MCInst &Inst, bool CallTarget,
- bool JumpTarget) const {
+ virtual void createBTI(MCInst &Inst, BTIKind BTI) const {
llvm_unreachable("not implemented");
}
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 031c16df5b159..9ec958ea79403 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -2775,15 +2775,18 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return Insts;
}
- void createBTI(MCInst &Inst, bool CallTarget,
- bool JumpTarget) const override {
+ void createBTI(MCInst &Inst, BTIKind BTI) const override {
Inst.setOpcode(AArch64::HINT);
+ Inst.clear();
+ bool CallTarget = BTI == BTIKind::C || BTI == BTIKind::JC;
+ bool JumpTarget = BTI == BTIKind::J || BTI == BTIKind::JC;
unsigned HintNum = getBTIHintNum(CallTarget, JumpTarget);
Inst.addOperand(MCOperand::createImm(HintNum));
}
- bool isBTILandingPad(MCInst &Inst, bool CallTarget,
- bool JumpTarget) const override {
+ bool isBTILandingPad(MCInst &Inst, BTIKind BTI) const override {
+ bool CallTarget = BTI == BTIKind::C || BTI == BTIKind::JC;
+ bool JumpTarget = BTI == BTIKind::J || BTI == BTIKind::JC;
unsigned HintNum = getBTIHintNum(CallTarget, JumpTarget);
bool IsExplicitBTI =
Inst.getOpcode() == AArch64::HINT && Inst.getNumOperands() == 1 &&
@@ -2800,22 +2803,14 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
Inst.getOpcode() == AArch64::PACIBSP;
}
- void updateBTIVariant(MCInst &Inst, bool CallTarget,
- bool JumpTarget) const override {
- assert(Inst.getOpcode() == AArch64::HINT && "Not a BTI instruction.");
- unsigned HintNum = getBTIHintNum(CallTarget, JumpTarget);
- Inst.clear();
- Inst.addOperand(MCOperand::createImm(HintNum));
- }
-
bool isCallCoveredByBTI(MCInst &Call, MCInst &Pad) const override {
assert((isIndirectCall(Call) || isIndirectBranch(Call)) &&
"Not an indirect call or branch.");
// A BLR can be accepted by a BTI c.
if (isIndirectCall(Call))
- return isBTILandingPad(Pad, true, false) ||
- isBTILandingPad(Pad, true, true);
+ return isBTILandingPad(Pad, BTIKind::C) ||
+ isBTILandingPad(Pad, BTIKind::JC);
// A BR can be accepted by a BTI j or BTI c (and BTI jc) IF the operand is
// x16 or x17. If the operand is not x16 or x17, it can be accepted by a BTI
@@ -2827,11 +2822,11 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
"Indirect branch does not have a register operand.");
MCPhysReg Reg = Call.getOperand(0).getReg();
if (Reg == AArch64::X16 || Reg == AArch64::X17)
- return isBTILandingPad(Pad, true, false) ||
- isBTILandingPad(Pad, false, true) ||
- isBTILandingPad(Pad, true, true);
- return isBTILandingPad(Pad, false, true) ||
- isBTILandingPad(Pad, true, true);
+ return isBTILandingPad(Pad, BTIKind::C) ||
+ isBTILandingPad(Pad, BTIKind::J) ||
+ isBTILandingPad(Pad, BTIKind::JC);
+ return isBTILandingPad(Pad, BTIKind::J) ||
+ isBTILandingPad(Pad, BTIKind::JC);
}
return false;
}
@@ -2846,11 +2841,11 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
if (isIndirectCall(Call)) {
// if we have a BTI j at the start, extend it to a BTI jc,
// otherwise insert a new BTI c.
- if (!Empty && isBTILandingPad(*II, false, true)) {
- updateBTIVariant(*II, true, true);
+ if (!Empty && isBTILandingPad(*II, BTIKind::J)) {
+ createBTI(*II, BTIKind::JC);
} else {
MCInst BTIInst;
- createBTI(BTIInst, true, false);
+ createBTI(BTIInst, BTIKind::C);
BB.insertInstruction(II, BTIInst);
}
}
@@ -2867,16 +2862,16 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
if (Reg == AArch64::X16 || Reg == AArch64::X17) {
// Add a new BTI c
MCInst BTIInst;
- createBTI(BTIInst, true, false);
+ createBTI(BTIInst, BTIKind::C);
BB.insertInstruction(II, BTIInst);
} else {
// If BB starts with a BTI c, extend it to BTI jc,
// otherwise insert a new BTI j.
- if (!Empty && isBTILandingPad(*II, true, false)) {
- updateBTIVariant(*II, true, true);
+ if (!Empty && isBTILandingPad(*II, BTIKind::C)) {
+ createBTI(*II, BTIKind::JC);
} else {
MCInst BTIInst;
- createBTI(BTIInst, false, true);
+ createBTI(BTIInst, BTIKind::J);
BB.insertInstruction(II, BTIInst);
}
}
diff --git a/bolt/unittests/Core/MCPlusBuilder.cpp b/bolt/unittests/Core/MCPlusBuilder.cpp
index e8323e87fe148..f41aeeb0dd672 100644
--- a/bolt/unittests/Core/MCPlusBuilder.cpp
+++ b/bolt/unittests/Core/MCPlusBuilder.cpp
@@ -150,51 +150,39 @@ TEST_P(MCPlusBuilderTester, AArch64_BTI) {
std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
MCInst BTIjc;
- BC->MIB->createBTI(BTIjc, true, true);
+ BC->MIB->createBTI(BTIjc, BTIKind::JC);
BB->addInstruction(BTIjc);
auto II = BB->begin();
ASSERT_EQ(II->getOpcode(), AArch64::HINT);
ASSERT_EQ(II->getOperand(0).getImm(), 38);
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, true));
- BC->MIB->updateBTIVariant(*II, true, false);
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::JC));
MCInst BTIj;
- BC->MIB->createBTI(BTIj, false, true);
+ BC->MIB->createBTI(BTIj, BTIKind::J);
II = BB->addInstruction(BTIj);
ASSERT_EQ(II->getOpcode(), AArch64::HINT);
ASSERT_EQ(II->getOperand(0).getImm(), 36);
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, false, true));
- BC->MIB->updateBTIVariant(*II, true, true);
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, true));
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::J));
MCInst BTIc;
- BC->MIB->createBTI(BTIc, true, false);
+ BC->MIB->createBTI(BTIc, BTIKind::C);
II = BB->addInstruction(BTIc);
ASSERT_EQ(II->getOpcode(), AArch64::HINT);
ASSERT_EQ(II->getOperand(0).getImm(), 34);
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
- BC->MIB->updateBTIVariant(*II, false, true);
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, false, true));
-
-#ifndef NDEBUG
- MCInst BTIinvalid;
- ASSERT_DEATH(BC->MIB->createBTI(BTIinvalid, false, false),
- "No target kinds!");
-#endif
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::C));
MCInst Paciasp = MCInstBuilder(AArch64::PACIASP);
II = BB->addInstruction(Paciasp);
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
- ASSERT_FALSE(BC->MIB->isBTILandingPad(*II, true, true));
- ASSERT_FALSE(BC->MIB->isBTILandingPad(*II, false, true));
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::C));
+ ASSERT_FALSE(BC->MIB->isBTILandingPad(*II, BTIKind::JC));
+ ASSERT_FALSE(BC->MIB->isBTILandingPad(*II, BTIKind::J));
ASSERT_TRUE(BC->MIB->isImplicitBTIC(*II));
MCInst Pacibsp = MCInstBuilder(AArch64::PACIBSP);
II = BB->addInstruction(Pacibsp);
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
- ASSERT_FALSE(BC->MIB->isBTILandingPad(*II, true, true));
- ASSERT_FALSE(BC->MIB->isBTILandingPad(*II, false, true));
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::C));
+ ASSERT_FALSE(BC->MIB->isBTILandingPad(*II, BTIKind::JC));
+ ASSERT_FALSE(BC->MIB->isBTILandingPad(*II, BTIKind::J));
ASSERT_TRUE(BC->MIB->isImplicitBTIC(*II));
}
@@ -207,7 +195,7 @@ TEST_P(MCPlusBuilderTester, AArch64_insertBTI_empty) {
BC->MIB->insertBTI(*BB, CallInst);
// Check that BTI c is added to the empty block.
auto II = BB->begin();
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::C));
}
TEST_P(MCPlusBuilderTester, AArch64_insertBTI_0) {
if (GetParam() != Triple::aarch64)
@@ -218,9 +206,11 @@ TEST_P(MCPlusBuilderTester, AArch64_insertBTI_0) {
BB->addInstruction(Inst);
// BR x16 needs BTI c or BTI j. We prefer adding a BTI c.
MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X16);
- BC->MIB->insertBTI(*BB, CallInst);
auto II = BB->begin();
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+ ASSERT_FALSE(BC->MIB->isCallCoveredByBTI(CallInst, *II));
+ BC->MIB->insertBTI(*BB, CallInst);
+ II = BB->begin();
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::C));
}
TEST_P(MCPlusBuilderTester, AArch64_insertBTI_1) {
@@ -229,13 +219,15 @@ TEST_P(MCPlusBuilderTester, AArch64_insertBTI_1) {
BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
MCInst BTIc;
- BC->MIB->createBTI(BTIc, true, false);
+ BC->MIB->createBTI(BTIc, BTIKind::C);
BB->addInstruction(BTIc);
// BR x16 needs BTI c or BTI j. We have a BTI c, no change is needed.
MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X16);
- BC->MIB->insertBTI(*BB, CallInst);
auto II = BB->begin();
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+ ASSERT_TRUE(BC->MIB->isCallCoveredByBTI(CallInst, *II));
+ BC->MIB->insertBTI(*BB, CallInst);
+ II = BB->begin();
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::C));
}
TEST_P(MCPlusBuilderTester, AArch64_insertBTI_2) {
@@ -244,14 +236,16 @@ TEST_P(MCPlusBuilderTester, AArch64_insertBTI_2) {
BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
MCInst BTIc;
- BC->MIB->createBTI(BTIc, true, false);
+ BC->MIB->createBTI(BTIc, BTIKind::C);
BB->addInstruction(BTIc);
// BR x5 needs BTI j
// we have BTI c -> extend it to BTI jc.
MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X5);
- BC->MIB->insertBTI(*BB, CallInst);
auto II = BB->begin();
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, true));
+ ASSERT_FALSE(BC->MIB->isCallCoveredByBTI(CallInst, *II));
+ BC->MIB->insertBTI(*BB, CallInst);
+ II = BB->begin();
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::JC));
}
TEST_P(MCPlusBuilderTester, AArch64_insertBTI_3) {
@@ -263,9 +257,11 @@ TEST_P(MCPlusBuilderTester, AArch64_insertBTI_3) {
BB->addInstruction(Inst);
// BR x5 needs BTI j
MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X5);
- BC->MIB->insertBTI(*BB, CallInst);
auto II = BB->begin();
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, false, true));
+ ASSERT_FALSE(BC->MIB->isCallCoveredByBTI(CallInst, *II));
+ BC->MIB->insertBTI(*BB, CallInst);
+ II = BB->begin();
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::J));
}
TEST_P(MCPlusBuilderTester, AArch64_insertBTI_4) {
@@ -277,9 +273,11 @@ TEST_P(MCPlusBuilderTester, AArch64_insertBTI_4) {
BB->addInstruction(Inst);
// BLR needs BTI c, regardless of the register used.
MCInst CallInst = MCInstBuilder(AArch64::BLR).addReg(AArch64::X5);
- BC->MIB->insertBTI(*BB, CallInst);
auto II = BB->begin();
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+ ASSERT_FALSE(BC->MIB->isCallCoveredByBTI(CallInst, *II));
+ BC->MIB->insertBTI(*BB, CallInst);
+ II = BB->begin();
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::C));
}
TEST_P(MCPlusBuilderTester, AArch64_insertBTI_5) {
@@ -288,14 +286,16 @@ TEST_P(MCPlusBuilderTester, AArch64_insertBTI_5) {
BinaryFunction *BF = BC->createInjectedBinaryFunction("BF", true);
std::unique_ptr<BinaryBasicBlock> BB = BF->createBasicBlock();
MCInst BTIj;
- BC->MIB->createBTI(BTIj, false, true);
+ BC->MIB->createBTI(BTIj, BTIKind::J);
BB->addInstruction(BTIj);
// BLR needs BTI c, regardless of the register used.
// We have a BTI j -> extend it to BTI jc.
MCInst CallInst = MCInstBuilder(AArch64::BLR).addReg(AArch64::X5);
- BC->MIB->insertBTI(*BB, CallInst);
auto II = BB->begin();
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, true));
+ ASSERT_FALSE(BC->MIB->isCallCoveredByBTI(CallInst, *II));
+ BC->MIB->insertBTI(*BB, CallInst);
+ II = BB->begin();
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::JC));
}
TEST_P(MCPlusBuilderTester, AArch64_insertBTI_6) {
@@ -308,9 +308,11 @@ TEST_P(MCPlusBuilderTester, AArch64_insertBTI_6) {
BB->addInstruction(Paciasp);
// PACI(AB)SP are implicit BTI c, no change needed.
MCInst CallInst = MCInstBuilder(AArch64::BR).addReg(AArch64::X17);
- BC->MIB->insertBTI(*BB, CallInst);
auto II = BB->begin();
- ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, true, false));
+ ASSERT_TRUE(BC->MIB->isCallCoveredByBTI(CallInst, *II));
+ BC->MIB->insertBTI(*BB, CallInst);
+ II = BB->begin();
+ ASSERT_TRUE(BC->MIB->isBTILandingPad(*II, BTIKind::C));
ASSERT_TRUE(BC->MIB->isPSignOnLR(*II));
}
|
peterwaller-arm
approved these changes
Dec 19, 2025
Member
peterwaller-arm
left a comment
peterwaller-arm
left a comment
There was a problem hiding this comment.
Thanks for following up, I think it does improve the clarity!
paschalis-mpeis
approved these changes
Dec 19, 2025
Member
paschalis-mpeis
left a comment
paschalis-mpeis
left a comment
There was a problem hiding this comment.
Nice refactoring indeed!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
are now unrepresentable.