Skip to content

Commit

Permalink
[BOLT] Refactor instruction creation interface. NFCI (llvm#85292)
Browse files Browse the repository at this point in the history 
Refactor MCPlusBuilder's create{Instruction}() functions that used to
return bool. We almost never check the return value as we rely on
llvm_unreachable() to detect unimplemented functionality. There were a
couple of cases that checked the return value, but they would hit the
unreachable condition first (at least in debug builds) before the return
value gets checked.
  • Loading branch information
@maksfb
maksfb authored Mar 14, 2024
1 parent ea429e1 commit bba790d
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 133 deletions.
52 changes: 14 additions & 38 deletions bolt/include/bolt/Core/MCPlusBuilder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -487,10 +487,9 @@ class MCPlusBuilder {
llvm_unreachable("not implemented");
}

virtual bool createDirectCall(MCInst &Inst, const MCSymbol *Target,
virtual void createDirectCall(MCInst &Inst, const MCSymbol *Target,
MCContext *Ctx, bool IsTailCall) {
llvm_unreachable("not implemented");
return false;
}

virtual MCPhysReg getX86R11() const { llvm_unreachable("not implemented"); }
Expand Down Expand Up @@ -1534,15 +1533,13 @@ class MCPlusBuilder {
}

/// Create a no-op instruction.
virtual bool createNoop(MCInst &Inst) const {
virtual void createNoop(MCInst &Inst) const {
llvm_unreachable("not implemented");
return false;
}

/// Create a return instruction.
virtual bool createReturn(MCInst &Inst) const {
virtual void createReturn(MCInst &Inst) const {
llvm_unreachable("not implemented");
return false;
}

/// Store \p Target absolute address to \p RegName
Expand All @@ -1556,32 +1553,23 @@ class MCPlusBuilder {

/// Creates a new unconditional branch instruction in Inst and set its operand
/// to TBB.
///
/// Returns true on success.
virtual bool createUncondBranch(MCInst &Inst, const MCSymbol *TBB,
virtual void createUncondBranch(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const {
llvm_unreachable("not implemented");
return false;
}

/// Creates a new call instruction in Inst and sets its operand to
/// Target.
///
/// Returns true on success.
virtual bool createCall(MCInst &Inst, const MCSymbol *Target,
virtual void createCall(MCInst &Inst, const MCSymbol *Target,
MCContext *Ctx) {
llvm_unreachable("not implemented");
return false;
}

/// Creates a new tail call instruction in Inst and sets its operand to
/// Target.
///
/// Returns true on success.
virtual bool createTailCall(MCInst &Inst, const MCSymbol *Target,
virtual void createTailCall(MCInst &Inst, const MCSymbol *Target,
MCContext *Ctx) {
llvm_unreachable("not implemented");
return false;
}

virtual void createLongTailCall(InstructionListType &Seq,
Expand All @@ -1590,43 +1578,36 @@ class MCPlusBuilder {
}

/// Creates a trap instruction in Inst.
///
/// Returns true on success.
virtual bool createTrap(MCInst &Inst) const {
virtual void createTrap(MCInst &Inst) const {
llvm_unreachable("not implemented");
return false;
}

/// Creates an instruction to bump the stack pointer just like a call.
virtual bool createStackPointerIncrement(MCInst &Inst, int Size = 8,
virtual void createStackPointerIncrement(MCInst &Inst, int Size = 8,
bool NoFlagsClobber = false) const {
llvm_unreachable("not implemented");
return false;
}

/// Creates an instruction to move the stack pointer just like a ret.
virtual bool createStackPointerDecrement(MCInst &Inst, int Size = 8,
virtual void createStackPointerDecrement(MCInst &Inst, int Size = 8,
bool NoFlagsClobber = false) const {
llvm_unreachable("not implemented");
return false;
}

/// Create a store instruction using \p StackReg as the base register
/// and \p Offset as the displacement.
virtual bool createSaveToStack(MCInst &Inst, const MCPhysReg &StackReg,
virtual void createSaveToStack(MCInst &Inst, const MCPhysReg &StackReg,
int Offset, const MCPhysReg &SrcReg,
int Size) const {
llvm_unreachable("not implemented");
return false;
}

virtual bool createLoad(MCInst &Inst, const MCPhysReg &BaseReg, int64_t Scale,
virtual void createLoad(MCInst &Inst, const MCPhysReg &BaseReg, int64_t Scale,
const MCPhysReg &IndexReg, int64_t Offset,
const MCExpr *OffsetExpr,
const MCPhysReg &AddrSegmentReg,
const MCPhysReg &DstReg, int Size) const {
llvm_unreachable("not implemented");
return false;
}

virtual InstructionListType createLoadImmediate(const MCPhysReg Dest,
Expand All @@ -1636,32 +1617,27 @@ class MCPlusBuilder {

/// Create a fragment of code (sequence of instructions) that load a 32-bit
/// address from memory, zero-extends it to 64 and jump to it (indirect jump).
virtual bool
virtual void
createIJmp32Frag(SmallVectorImpl<MCInst> &Insts, const MCOperand &BaseReg,
const MCOperand &Scale, const MCOperand &IndexReg,
const MCOperand &Offset, const MCOperand &TmpReg) const {
llvm_unreachable("not implemented");
return false;
}

/// Create a load instruction using \p StackReg as the base register
/// and \p Offset as the displacement.
virtual bool createRestoreFromStack(MCInst &Inst, const MCPhysReg &StackReg,
virtual void createRestoreFromStack(MCInst &Inst, const MCPhysReg &StackReg,
int Offset, const MCPhysReg &DstReg,
int Size) const {
llvm_unreachable("not implemented");
return false;
}

/// Creates a call frame pseudo instruction. A single operand identifies which
/// MCCFIInstruction this MCInst is referring to.
///
/// Returns true on success.
virtual bool createCFI(MCInst &Inst, int64_t Offset) const {
virtual void createCFI(MCInst &Inst, int64_t Offset) const {
Inst.clear();
Inst.setOpcode(TargetOpcode::CFI_INSTRUCTION);
Inst.addOperand(MCOperand::createImm(Offset));
return true;
}

/// Create an inline version of memcpy(dest, src, 1).
Expand Down
7 changes: 3 additions & 4 deletions bolt/lib/Passes/BinaryPasses.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1056,10 +1056,9 @@ void Peepholes::addTailcallTraps(BinaryFunction &Function) {
MCInst *Inst = BB.getLastNonPseudoInstr();
if (Inst && MIB->isTailCall(*Inst) && MIB->isIndirectBranch(*Inst)) {
MCInst Trap;
if (MIB->createTrap(Trap)) {
BB.addInstruction(Trap);
++TailCallTraps;
}
MIB->createTrap(Trap);
BB.addInstruction(Trap);
++TailCallTraps;
}
}
}
Expand Down
44 changes: 15 additions & 29 deletions bolt/lib/Passes/ShrinkWrapping.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -680,16 +680,15 @@ void StackLayoutModifier::performChanges() {
if (StackPtrReg != BC.MIB->getFramePointer())
Adjustment = -Adjustment;
if (IsLoad)
Success = BC.MIB->createRestoreFromStack(
Inst, StackPtrReg, StackOffset + Adjustment, Reg, Size);
BC.MIB->createRestoreFromStack(Inst, StackPtrReg,
StackOffset + Adjustment, Reg, Size);
else if (IsStore)
Success = BC.MIB->createSaveToStack(
Inst, StackPtrReg, StackOffset + Adjustment, Reg, Size);
BC.MIB->createSaveToStack(Inst, StackPtrReg, StackOffset + Adjustment,
Reg, Size);
LLVM_DEBUG({
dbgs() << "Adjusted instruction: ";
Inst.dump();
});
assert(Success);
}
}
}
Expand Down Expand Up @@ -1653,19 +1652,13 @@ Expected<MCInst> ShrinkWrapping::createStackAccess(int SPVal, int FPVal,
if (SPVal != StackPointerTracking::SUPERPOSITION &&
SPVal != StackPointerTracking::EMPTY) {
if (FIE.IsLoad) {
if (!BC.MIB->createRestoreFromStack(NewInst, BC.MIB->getStackPointer(),
FIE.StackOffset - SPVal, FIE.RegOrImm,
FIE.Size)) {
return createFatalBOLTError(
"createRestoreFromStack: not supported on this platform\n");
}
} else {
if (!BC.MIB->createSaveToStack(NewInst, BC.MIB->getStackPointer(),
BC.MIB->createRestoreFromStack(NewInst, BC.MIB->getStackPointer(),
FIE.StackOffset - SPVal, FIE.RegOrImm,
FIE.Size)) {
return createFatalBOLTError(
"createSaveToStack: not supported on this platform\n");
}
FIE.Size);
} else {
BC.MIB->createSaveToStack(NewInst, BC.MIB->getStackPointer(),
FIE.StackOffset - SPVal, FIE.RegOrImm,
FIE.Size);
}
if (CreatePushOrPop)
BC.MIB->changeToPushOrPop(NewInst);
Expand All @@ -1675,19 +1668,12 @@ Expected<MCInst> ShrinkWrapping::createStackAccess(int SPVal, int FPVal,
FPVal != StackPointerTracking::EMPTY);

if (FIE.IsLoad) {
if (!BC.MIB->createRestoreFromStack(NewInst, BC.MIB->getFramePointer(),
FIE.StackOffset - FPVal, FIE.RegOrImm,
FIE.Size)) {
return createFatalBOLTError(
"createRestoreFromStack: not supported on this platform\n");
}
} else {
if (!BC.MIB->createSaveToStack(NewInst, BC.MIB->getFramePointer(),
BC.MIB->createRestoreFromStack(NewInst, BC.MIB->getFramePointer(),
FIE.StackOffset - FPVal, FIE.RegOrImm,
FIE.Size)) {
return createFatalBOLTError(
"createSaveToStack: not supported on this platform\n");
}
FIE.Size);
} else {
BC.MIB->createSaveToStack(NewInst, BC.MIB->getFramePointer(),
FIE.StackOffset - FPVal, FIE.RegOrImm, FIE.Size);
}
return NewInst;
}
Expand Down
23 changes: 8 additions & 15 deletions bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1026,7 +1026,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return Code;
}

bool createTailCall(MCInst &Inst, const MCSymbol *Target,
void createTailCall(MCInst &Inst, const MCSymbol *Target,
MCContext *Ctx) override {
return createDirectCall(Inst, Target, Ctx, /*IsTailCall*/ true);
}
Expand All @@ -1036,11 +1036,10 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
createShortJmp(Seq, Target, Ctx, /*IsTailCall*/ true);
}

bool createTrap(MCInst &Inst) const override {
void createTrap(MCInst &Inst) const override {
Inst.clear();
Inst.setOpcode(AArch64::BRK);
Inst.addOperand(MCOperand::createImm(1));
return true;
}

bool convertJmpToTailCall(MCInst &Inst) override {
Expand Down Expand Up @@ -1068,16 +1067,15 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
Inst.getOperand(0).getImm() == 0;
}

bool createNoop(MCInst &Inst) const override {
void createNoop(MCInst &Inst) const override {
Inst.setOpcode(AArch64::HINT);
Inst.clear();
Inst.addOperand(MCOperand::createImm(0));
return true;
}

bool mayStore(const MCInst &Inst) const override { return false; }

bool createDirectCall(MCInst &Inst, const MCSymbol *Target, MCContext *Ctx,
void createDirectCall(MCInst &Inst, const MCSymbol *Target, MCContext *Ctx,
bool IsTailCall) override {
Inst.setOpcode(IsTailCall ? AArch64::B : AArch64::BL);
Inst.clear();
Expand All @@ -1086,7 +1084,6 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
*Ctx, 0)));
if (IsTailCall)
convertJmpToTailCall(Inst);
return true;
}

bool analyzeBranch(InstructionIterator Begin, InstructionIterator End,
Expand Down Expand Up @@ -1293,14 +1290,13 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return true;
}

bool createUncondBranch(MCInst &Inst, const MCSymbol *TBB,
void createUncondBranch(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const override {
Inst.setOpcode(AArch64::B);
Inst.clear();
Inst.addOperand(MCOperand::createExpr(getTargetExprFor(
Inst, MCSymbolRefExpr::create(TBB, MCSymbolRefExpr::VK_None, *Ctx),
*Ctx, 0)));
return true;
}

bool shouldRecordCodeRelocation(uint64_t RelType) const override {
Expand Down Expand Up @@ -1353,14 +1349,13 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return StringRef("\0\0\0\0", 4);
}

bool createReturn(MCInst &Inst) const override {
void createReturn(MCInst &Inst) const override {
Inst.setOpcode(AArch64::RET);
Inst.clear();
Inst.addOperand(MCOperand::createReg(AArch64::LR));
return true;
}

bool createStackPointerIncrement(
void createStackPointerIncrement(
MCInst &Inst, int Size,
bool NoFlagsClobber = false /*unused for AArch64*/) const override {
Inst.setOpcode(AArch64::SUBXri);
Expand All @@ -1369,10 +1364,9 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
Inst.addOperand(MCOperand::createReg(AArch64::SP));
Inst.addOperand(MCOperand::createImm(Size));
Inst.addOperand(MCOperand::createImm(0));
return true;
}

bool createStackPointerDecrement(
void createStackPointerDecrement(
MCInst &Inst, int Size,
bool NoFlagsClobber = false /*unused for AArch64*/) const override {
Inst.setOpcode(AArch64::ADDXri);
Expand All @@ -1381,7 +1375,6 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
Inst.addOperand(MCOperand::createReg(AArch64::SP));
Inst.addOperand(MCOperand::createImm(Size));
Inst.addOperand(MCOperand::createImm(0));
return true;
}

void createIndirectBranch(MCInst &Inst, MCPhysReg MemBaseReg,
Expand Down
13 changes: 5 additions & 8 deletions bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,46 +219,43 @@ class RISCVMCPlusBuilder : public MCPlusBuilder {
return true;
}

bool createReturn(MCInst &Inst) const override {
void createReturn(MCInst &Inst) const override {
// TODO "c.jr ra" when RVC is enabled
Inst.setOpcode(RISCV::JALR);
Inst.clear();
Inst.addOperand(MCOperand::createReg(RISCV::X0));
Inst.addOperand(MCOperand::createReg(RISCV::X1));
Inst.addOperand(MCOperand::createImm(0));
return true;
}

bool createUncondBranch(MCInst &Inst, const MCSymbol *TBB,
void createUncondBranch(MCInst &Inst, const MCSymbol *TBB,
MCContext *Ctx) const override {
Inst.setOpcode(RISCV::JAL);
Inst.clear();
Inst.addOperand(MCOperand::createReg(RISCV::X0));
Inst.addOperand(MCOperand::createExpr(
MCSymbolRefExpr::create(TBB, MCSymbolRefExpr::VK_None, *Ctx)));
return true;
}

StringRef getTrapFillValue() const override {
return StringRef("\0\0\0\0", 4);
}

bool createCall(unsigned Opcode, MCInst &Inst, const MCSymbol *Target,
void createCall(unsigned Opcode, MCInst &Inst, const MCSymbol *Target,
MCContext *Ctx) {
Inst.setOpcode(Opcode);
Inst.clear();
Inst.addOperand(MCOperand::createExpr(RISCVMCExpr::create(
MCSymbolRefExpr::create(Target, MCSymbolRefExpr::VK_None, *Ctx),
RISCVMCExpr::VK_RISCV_CALL, *Ctx)));
return true;
}

bool createCall(MCInst &Inst, const MCSymbol *Target,
void createCall(MCInst &Inst, const MCSymbol *Target,
MCContext *Ctx) override {
return createCall(RISCV::PseudoCALL, Inst, Target, Ctx);
}

bool createTailCall(MCInst &Inst, const MCSymbol *Target,
void createTailCall(MCInst &Inst, const MCSymbol *Target,
MCContext *Ctx) override {
return createCall(RISCV::PseudoTAIL, Inst, Target, Ctx);
}
Expand Down
Loading

0 comments on commit bba790d

Please sign in to comment.