Skip to content

Commit a41b149

Browse files
[MachineInstr] add insert method for variadic instructions (llvm#67699)
As alluded to in llvm#20571, it would be nice if we could mutate operand lists of MachineInstr's more safely. Add an insert method that together with removeOperand allows for easier splicing of operands. Splitting this patch off early to get feedback; I need to either: - mutate an INLINEASM{_BR} MachinInstr's MachineOperands from being registers (physical or virtual) to memory (MachineOperandType::MO_FrameIndex). These are not 1:1 operand replacements, but N:M operand replacements. i.e. we need to update 2 MachineOperands into the middle of the operand list to 5 (at least for x86_64). - copy, modify, write a new MachineInstr which has its relevant operands replaced. Either approaches are hazarded by existing references to either the operands being moved, or the instruction being removed+replaced. For my purposes in regalloc, either seem to work for me, so hopefully reviewers can help me determine which approach is preferable. The second would involve no new methods on MachineInstr. One question I had while looking at this was: "why does MachineInstr have BOTH a NumOperands member AND a MCInstrDesc member that itself has a NumOperands member? How many operands can a MachineInstr have? Do I need to update BOTH (keeping them in sync)?" FWICT, only "variadic" MachineInstrs have MCInstrDesc with NumOperands (of the MCInstrDesc) set to zero. If the MCInstrDesc's NumOperands is non-zero, then the NumOperands on the MachineInstr itself cannot exceed this value (IIUC) else an assert will be triggered. For most non-psuedo instructions (or at least non-varidic instructions), insert is less likely to be useful. To run the newly added unittest: $ pushd llvm/build; ninja CodeGenTests; popd $ ./llvm/build/unittests/CodeGen/CodeGenTests \ --gtest_filter=MachineInstrTest.SpliceOperands This is meant to mirror `MCInst::insert`.
1 parent c118339 commit a41b149

File tree

3 files changed

+131
-0
lines changed

3 files changed

+131
-0
lines changed

llvm/include/llvm/CodeGen/MachineInstr.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1812,6 +1812,9 @@ class MachineInstr
18121812
/// preferred.
18131813
void addOperand(const MachineOperand &Op);
18141814

1815+
/// Inserts Ops BEFORE It. Can untie/retie tied operands.
1816+
void insert(mop_iterator InsertBefore, ArrayRef<MachineOperand> Ops);
1817+
18151818
/// Replace the instruction descriptor (thus opcode) of
18161819
/// the current instruction with a new one.
18171820
void setDesc(const MCInstrDesc &TID);

llvm/lib/CodeGen/MachineInstr.cpp

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2481,3 +2481,48 @@ MachineInstr::getFirst5RegLLTs() const {
24812481
Reg2, getRegInfo()->getType(Reg2), Reg3, getRegInfo()->getType(Reg3),
24822482
Reg4, getRegInfo()->getType(Reg4));
24832483
}
2484+
2485+
void MachineInstr::insert(mop_iterator InsertBefore,
2486+
ArrayRef<MachineOperand> Ops) {
2487+
assert(InsertBefore != nullptr && "invalid iterator");
2488+
assert(InsertBefore->getParent() == this &&
2489+
"iterator points to operand of other inst");
2490+
if (Ops.empty())
2491+
return;
2492+
2493+
// Do one pass to untie operands.
2494+
SmallDenseMap<unsigned, unsigned> TiedOpIndices;
2495+
for (const MachineOperand &MO : operands()) {
2496+
if (MO.isReg() && MO.isTied()) {
2497+
unsigned OpNo = getOperandNo(&MO);
2498+
unsigned TiedTo = findTiedOperandIdx(OpNo);
2499+
TiedOpIndices[OpNo] = TiedTo;
2500+
untieRegOperand(OpNo);
2501+
}
2502+
}
2503+
2504+
unsigned OpIdx = getOperandNo(InsertBefore);
2505+
unsigned NumOperands = getNumOperands();
2506+
unsigned OpsToMove = NumOperands - OpIdx;
2507+
2508+
SmallVector<MachineOperand> MovingOps;
2509+
MovingOps.reserve(OpsToMove);
2510+
2511+
for (unsigned I = 0; I < OpsToMove; ++I) {
2512+
MovingOps.emplace_back(getOperand(OpIdx));
2513+
removeOperand(OpIdx);
2514+
}
2515+
for (const MachineOperand &MO : Ops)
2516+
addOperand(MO);
2517+
for (const MachineOperand &OpMoved : MovingOps)
2518+
addOperand(OpMoved);
2519+
2520+
// Re-tie operands.
2521+
for (auto [Tie1, Tie2] : TiedOpIndices) {
2522+
if (Tie1 >= OpIdx)
2523+
Tie1 += Ops.size();
2524+
if (Tie2 >= OpIdx)
2525+
Tie2 += Ops.size();
2526+
tieOperands(Tie1, Tie2);
2527+
}
2528+
}

llvm/unittests/CodeGen/MachineInstrTest.cpp

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,4 +478,87 @@ TEST(MachineInstrBuilder, BuildMI) {
478478

479479
static_assert(std::is_trivially_copyable_v<MCOperand>, "trivially copyable");
480480

481+
TEST(MachineInstrTest, SpliceOperands) {
482+
LLVMContext Ctx;
483+
Module Mod("Module", Ctx);
484+
std::unique_ptr<MachineFunction> MF = createMachineFunction(Ctx, Mod);
485+
MachineBasicBlock *MBB = MF->CreateMachineBasicBlock();
486+
MCInstrDesc MCID = {TargetOpcode::INLINEASM,
487+
0,
488+
0,
489+
0,
490+
0,
491+
0,
492+
0,
493+
0,
494+
0,
495+
(1ULL << MCID::Pseudo) | (1ULL << MCID::Variadic),
496+
0};
497+
MachineInstr *MI = MF->CreateMachineInstr(MCID, DebugLoc());
498+
MBB->insert(MBB->begin(), MI);
499+
MI->addOperand(MachineOperand::CreateImm(0));
500+
MI->addOperand(MachineOperand::CreateImm(1));
501+
MI->addOperand(MachineOperand::CreateImm(2));
502+
MI->addOperand(MachineOperand::CreateImm(3));
503+
MI->addOperand(MachineOperand::CreateImm(4));
504+
505+
MI->removeOperand(1);
506+
EXPECT_EQ(MI->getOperand(1).getImm(), MachineOperand::CreateImm(2).getImm());
507+
EXPECT_EQ(MI->getNumOperands(), 4U);
508+
509+
MachineOperand Ops[] = {
510+
MachineOperand::CreateImm(42), MachineOperand::CreateImm(1024),
511+
MachineOperand::CreateImm(2048), MachineOperand::CreateImm(4096),
512+
MachineOperand::CreateImm(8192),
513+
};
514+
auto *It = MI->operands_begin();
515+
++It;
516+
MI->insert(It, Ops);
517+
518+
EXPECT_EQ(MI->getNumOperands(), 9U);
519+
EXPECT_EQ(MI->getOperand(0).getImm(), MachineOperand::CreateImm(0).getImm());
520+
EXPECT_EQ(MI->getOperand(1).getImm(), MachineOperand::CreateImm(42).getImm());
521+
EXPECT_EQ(MI->getOperand(2).getImm(),
522+
MachineOperand::CreateImm(1024).getImm());
523+
EXPECT_EQ(MI->getOperand(3).getImm(),
524+
MachineOperand::CreateImm(2048).getImm());
525+
EXPECT_EQ(MI->getOperand(4).getImm(),
526+
MachineOperand::CreateImm(4096).getImm());
527+
EXPECT_EQ(MI->getOperand(5).getImm(),
528+
MachineOperand::CreateImm(8192).getImm());
529+
EXPECT_EQ(MI->getOperand(6).getImm(), MachineOperand::CreateImm(2).getImm());
530+
EXPECT_EQ(MI->getOperand(7).getImm(), MachineOperand::CreateImm(3).getImm());
531+
EXPECT_EQ(MI->getOperand(8).getImm(), MachineOperand::CreateImm(4).getImm());
532+
533+
// test tied operands
534+
MCRegisterClass MRC{0, 0, 0, 0, 0, 0, 0, 0, /*Allocatable=*/true};
535+
TargetRegisterClass RC{&MRC, 0, 0, {}, 0, 0, 0, 0, 0, 0, 0};
536+
// MachineRegisterInfo will be very upset if these registers aren't
537+
// allocatable.
538+
assert(RC.isAllocatable() && "unusable TargetRegisterClass");
539+
MachineRegisterInfo &MRI = MF->getRegInfo();
540+
Register A = MRI.createVirtualRegister(&RC);
541+
Register B = MRI.createVirtualRegister(&RC);
542+
MI->getOperand(0).ChangeToRegister(A, /*isDef=*/true);
543+
MI->getOperand(1).ChangeToRegister(B, /*isDef=*/false);
544+
MI->tieOperands(0, 1);
545+
EXPECT_TRUE(MI->getOperand(0).isTied());
546+
EXPECT_TRUE(MI->getOperand(1).isTied());
547+
EXPECT_EQ(MI->findTiedOperandIdx(0), 1U);
548+
EXPECT_EQ(MI->findTiedOperandIdx(1), 0U);
549+
MI->insert(&MI->getOperand(1), {MachineOperand::CreateImm(7)});
550+
EXPECT_TRUE(MI->getOperand(0).isTied());
551+
EXPECT_TRUE(MI->getOperand(1).isImm());
552+
EXPECT_TRUE(MI->getOperand(2).isTied());
553+
EXPECT_EQ(MI->findTiedOperandIdx(0), 2U);
554+
EXPECT_EQ(MI->findTiedOperandIdx(2), 0U);
555+
EXPECT_EQ(MI->getOperand(0).getReg(), A);
556+
EXPECT_EQ(MI->getOperand(2).getReg(), B);
557+
558+
// bad inputs
559+
EXPECT_EQ(MI->getNumOperands(), 10U);
560+
MI->insert(MI->operands_begin(), {});
561+
EXPECT_EQ(MI->getNumOperands(), 10U);
562+
}
563+
481564
} // end namespace

0 commit comments

Comments
 (0)