[BOLT][NFC] Infailable fns return void #92018
Merged
Conversation
This file contains 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
urnathan
requested review from
aaupov,
maksfb,
rafaelauler,
ayermolo and
dcci
as code owners
|
@llvm/pr-subscribers-bolt Author: Nathan Sidwell (urnathan) ChangesBOLT's MCPlusBuilder's Thus, just return nothing and avoid confusing implementors. I discovered this issue by having a While there, Full diff: https://github.com/llvm/llvm-project/pull/92018.diff 5 Files Affected:
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; }
|
urnathan
added a commit
that referenced
this pull request
May 17, 2024
`isUnsupportedBranch` is not a very informative name, and doesn't match its corresponding `reverseBranchCondition`, as I noted in PR #92018. Here's a renaming to a more mnemonic name.
urnathan
force-pushed
the
push/reversebranch
branch
from
bc0dd98
to
bcb1ec8
Compare
rafaelauler
approved these changes
May 22, 2024
Closed
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.
BOLT's MCPlusBuilder's
reverseBranchConditionandreplaceBranchTargethooks 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
reverseBranchConditionimplementation that could return false, and things went sideways.While there,
isUnsupportedBranchis the predicate as to whetherreverseBranchConditioncan be applied, but (a) its name is not very related, and (b) its sense is inverted. Perhaps replacing withisReversibleBranchmight be in order?