[llvm] [BOLT][NFC] Infailable fns return void (PR #92018)
via llvm-commits
llvm-commits at lists.llvm.org
Tue May 14 06:16:31 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-bolt
Author: Nathan Sidwell (urnathan)
<details>
<summary>Changes</summary>
BOLT's MCPlusBuilder's `reverseBranchCondition` and `replaceBranchTarget` hooks both return a success boolean. Except all-but-one caller does not check for success -- the one the does check turns failure into a fatal error anway. The 3 implementations always succeed (unsurprisingly I suppose, given the caller assumption of success).
Thus, just return nothing and avoid confusing implementors.
I discovered this issue by having a `reverseBranchCondition` implementation that could return false, and things went sideways.
While there, `isUnsupportedBranch` is the predicate as to whether `reverseBranchCondition` can be applied, but (a) its name is not very related, and (b) its sense is inverted. Perhaps replacing with `isReversibleBranch` might be in order?
---
Full diff: https://github.com/llvm/llvm-project/pull/92018.diff
5 Files Affected:
- (modified) bolt/include/bolt/Core/MCPlusBuilder.h (+2-8)
- (modified) bolt/lib/Passes/VeneerElimination.cpp (+2-5)
- (modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (+3-4)
- (modified) bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp (+3-4)
- (modified) bolt/lib/Target/X86/X86MCPlusBuilder.cpp (+2-4)
``````````diff
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index f7614cf9ac977..5172b41a4da13 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -1706,12 +1706,9 @@ class MCPlusBuilder {
}
/// Reverses the branch condition in Inst and update its taken target to TBB.
- ///
- /// Returns true on success.
- virtual bool reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
+ virtual void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const {
llvm_unreachable("not implemented");
- return false;
}
virtual bool replaceBranchCondition(MCInst &Inst, const MCSymbol *TBB,
@@ -1751,12 +1748,9 @@ class MCPlusBuilder {
}
/// Sets the taken target of the branch instruction to Target.
- ///
- /// Returns true on success.
- virtual bool replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB,
+ virtual void replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const {
llvm_unreachable("not implemented");
- return false;
}
/// Extract a symbol and an addend out of the fixup value expression.
diff --git a/bolt/lib/Passes/VeneerElimination.cpp b/bolt/lib/Passes/VeneerElimination.cpp
index 0bec11128c7ce..87fe625e8c3b3 100644
--- a/bolt/lib/Passes/VeneerElimination.cpp
+++ b/bolt/lib/Passes/VeneerElimination.cpp
@@ -77,11 +77,8 @@ Error VeneerElimination::runOnFunctions(BinaryContext &BC) {
continue;
VeneerCallers++;
- if (!BC.MIB->replaceBranchTarget(
- Instr, VeneerDestinations[TargetSymbol], BC.Ctx.get())) {
- return createFatalBOLTError(
- "BOLT-ERROR: updating veneer call destination failed\n");
- }
+ BC.MIB->replaceBranchTarget(Instr, VeneerDestinations[TargetSymbol],
+ BC.Ctx.get());
}
}
}
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 0ae9d3668b93b..a74eda8e4a566 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -616,7 +616,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return getTargetAddend(Op.getExpr());
}
- bool replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB,
+ void replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const override {
assert((isCall(Inst) || isBranch(Inst)) && !isIndirectBranch(Inst) &&
"Invalid instruction");
@@ -638,7 +638,6 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
*OI = MCOperand::createExpr(
MCSymbolRefExpr::create(TBB, MCSymbolRefExpr::VK_None, *Ctx));
- return true;
}
/// Matches indirect branch patterns in AArch64 related to a jump table (JT),
@@ -969,7 +968,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
}
}
- bool reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
+ void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const override {
if (isTB(Inst) || isCB(Inst)) {
Inst.setOpcode(getInvertedBranchOpcode(Inst.getOpcode()));
@@ -984,7 +983,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
LLVM_DEBUG(Inst.dump());
llvm_unreachable("Unrecognized branch instruction");
}
- return replaceBranchTarget(Inst, TBB, Ctx);
+ replaceBranchTarget(Inst, TBB, Ctx);
}
int getPCRelEncodingSize(const MCInst &Inst) const override {
diff --git a/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp b/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp
index 74f2f0aae91e6..eb3f38a0b8f4a 100644
--- a/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp
+++ b/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp
@@ -151,14 +151,14 @@ class RISCVMCPlusBuilder : public MCPlusBuilder {
}
}
- bool reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
+ void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const override {
auto Opcode = getInvertedBranchOpcode(Inst.getOpcode());
Inst.setOpcode(Opcode);
- return replaceBranchTarget(Inst, TBB, Ctx);
+ replaceBranchTarget(Inst, TBB, Ctx);
}
- bool replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB,
+ void replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const override {
assert((isCall(Inst) || isBranch(Inst)) && !isIndirectBranch(Inst) &&
"Invalid instruction");
@@ -170,7 +170,6 @@ class RISCVMCPlusBuilder : public MCPlusBuilder {
Inst.getOperand(SymOpIndex) = MCOperand::createExpr(
MCSymbolRefExpr::create(TBB, MCSymbolRefExpr::VK_None, *Ctx));
- return true;
}
IndirectBranchType analyzeIndirectBranch(
diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
index 8b1894953f375..afe4a167c8acc 100644
--- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -2782,14 +2782,13 @@ class X86MCPlusBuilder : public MCPlusBuilder {
Inst.addOperand(MCOperand::createImm(CC));
}
- bool reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
+ void reverseBranchCondition(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const override {
unsigned InvCC = getInvertedCondCode(getCondCode(Inst));
assert(InvCC != X86::COND_INVALID && "invalid branch instruction");
Inst.getOperand(Info->get(Inst.getOpcode()).NumOperands - 1).setImm(InvCC);
Inst.getOperand(0) = MCOperand::createExpr(
MCSymbolRefExpr::create(TBB, MCSymbolRefExpr::VK_None, *Ctx));
- return true;
}
bool replaceBranchCondition(MCInst &Inst, const MCSymbol *TBB, MCContext *Ctx,
@@ -2832,13 +2831,12 @@ class X86MCPlusBuilder : public MCPlusBuilder {
}
}
- bool replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB,
+ void replaceBranchTarget(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const override {
assert((isCall(Inst) || isBranch(Inst)) && !isIndirectBranch(Inst) &&
"Invalid instruction");
Inst.getOperand(0) = MCOperand::createExpr(
MCSymbolRefExpr::create(TBB, MCSymbolRefExpr::VK_None, *Ctx));
- return true;
}
MCPhysReg getX86R11() const override { return X86::R11; }
``````````
</details>
https://github.com/llvm/llvm-project/pull/92018
More information about the llvm-commits
mailing list