Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MachineInstr] add insert method for variadic instructions #67699

Merged
merged 11 commits into from
Oct 30, 2023
Merged

[MachineInstr] add insert method for variadic instructions #67699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ff094aa
[MachineInstr] add insert method for variadic instructions
nickdesaulniers Sep 28, 2023
eb410ae
fix tied operands insertion
nickdesaulniers Oct 17, 2023
e5ed345
add tied ops test
nickdesaulniers Oct 27, 2023
92d53e9
test bad inputs
nickdesaulniers Oct 27, 2023
6ec4178
s/DenseMap/SmallDenseMap/
nickdesaulniers Oct 30, 2023
381dd4c
remove const
nickdesaulniers Oct 30, 2023
4a862dd
remove support for null iterator
nickdesaulniers Oct 30, 2023
69b94e3
move assert earlier
nickdesaulniers Oct 30, 2023
0d7d924
git clang-format HEAD~8
nickdesaulniers Oct 30, 2023
8449626
make sure registerclass is allocatable before changing operands
nickdesaulniers Oct 30, 2023
6ba0e06
rename parameter
nickdesaulniers Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions llvm/include/llvm/CodeGen/MachineInstr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1818,6 +1818,9 @@ class MachineInstr
/// preferred.
void addOperand(const MachineOperand &Op);

/// Inserts Ops BEFORE It. Can untie/retie tied operands.
void insert(mop_iterator InsertBefore, ArrayRef<MachineOperand> Ops);

/// Replace the instruction descriptor (thus opcode) of
/// the current instruction with a new one.
void setDesc(const MCInstrDesc &TID) { MCID = &TID; }
Expand Down
45 changes: 45 additions & 0 deletions llvm/lib/CodeGen/MachineInstr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2475,3 +2475,48 @@ MachineInstr::getFirst5RegLLTs() const {
Reg2, getRegInfo()->getType(Reg2), Reg3, getRegInfo()->getType(Reg3),
Reg4, getRegInfo()->getType(Reg4));
}

void MachineInstr::insert(mop_iterator InsertBefore,
ArrayRef<MachineOperand> Ops) {
assert(InsertBefore != nullptr && "invalid iterator");
assert(InsertBefore->getParent() == this &&
"iterator points to operand of other inst");
if (Ops.empty())
return;

// Do one pass to untie operands.
SmallDenseMap<unsigned, unsigned> TiedOpIndices;
for (const MachineOperand &MO : operands()) {
if (MO.isReg() && MO.isTied()) {
unsigned OpNo = getOperandNo(&MO);
unsigned TiedTo = findTiedOperandIdx(OpNo);
TiedOpIndices[OpNo] = TiedTo;
untieRegOperand(OpNo);
}
}

unsigned OpIdx = getOperandNo(InsertBefore);
unsigned NumOperands = getNumOperands();
unsigned OpsToMove = NumOperands - OpIdx;

SmallVector<MachineOperand> MovingOps;
MovingOps.reserve(OpsToMove);

for (unsigned I = 0; I < OpsToMove; ++I) {
MovingOps.emplace_back(getOperand(OpIdx));
removeOperand(OpIdx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe do this in reverse order so we waste less cycles on the moveOperands() call within MachineInstr::removeOperand(). On that note I wonder if moveOperands() would work here as well instead of the removeOperand / addOperand for MovingOps here?

Admittedly it looks like moveOperands() doesn't really adjust the TiedTo markers so that's why it wouldn't work here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On that note I wonder if moveOperands() would work here as well instead of the removeOperand / addOperand for MovingOps here?

The issue there is we would miss the calls to MachineFunction::allocateOperandArray from MachineInstr::addOperand. Since splice grows the operand list, we need to realloc the storage for the operands. MachineInstr::moveOperands just memmoves operands around which is not safe to do when the backing store hasn't been grown.

Admittedly it looks like moveOperands() doesn't really adjust the TiedTo markers so that's why it wouldn't work here?

That's also an issue, though I'm manually handling tied operands.

Maybe do this in reverse order so we waste less cycles on the moveOperands() call within MachineInstr::removeOperand().

I don't think there are any cycles to be saved? Perhaps if splice is being used to append operands on the end? Usually you splice operands into the middle, so you need to memmove the elements occurring after the insertion point multiple times.

Notice in this loop that the induction variable I is not being used as an index; we're removing OpIdx repeatedly as this is slicing off the elements past the insertion point.

Later, we need to re-add the operands back in the correct order (though it's not hard to reverse the loop below).

Did I perhaps miss something though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense. I haven't looked at the code as deeply as you have; just wanted to ask the questions to make sure :)

}
for (const MachineOperand &MO : Ops)
nickdesaulniers marked this conversation as resolved.
Show resolved Hide resolved
addOperand(MO);
for (const MachineOperand &OpMoved : MovingOps)
addOperand(OpMoved);

// Re-tie operands.
for (auto [Tie1, Tie2] : TiedOpIndices) {
if (Tie1 >= OpIdx)
Tie1 += Ops.size();
if (Tie2 >= OpIdx)
Tie2 += Ops.size();
tieOperands(Tie1, Tie2);
}
}
83 changes: 83 additions & 0 deletions llvm/unittests/CodeGen/MachineInstrTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,4 +478,87 @@ TEST(MachineInstrBuilder, BuildMI) {

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

TEST(MachineInstrTest, SpliceOperands) {
LLVMContext Ctx;
Module Mod("Module", Ctx);
std::unique_ptr<MachineFunction> MF = createMachineFunction(Ctx, Mod);
MachineBasicBlock *MBB = MF->CreateMachineBasicBlock();
MCInstrDesc MCID = {TargetOpcode::INLINEASM,
0,
0,
0,
0,
0,
0,
0,
0,
(1ULL << MCID::Pseudo) | (1ULL << MCID::Variadic),
0};
MachineInstr *MI = MF->CreateMachineInstr(MCID, DebugLoc());
MBB->insert(MBB->begin(), MI);
MI->addOperand(MachineOperand::CreateImm(0));
MI->addOperand(MachineOperand::CreateImm(1));
MI->addOperand(MachineOperand::CreateImm(2));
MI->addOperand(MachineOperand::CreateImm(3));
MI->addOperand(MachineOperand::CreateImm(4));

MI->removeOperand(1);
EXPECT_EQ(MI->getOperand(1).getImm(), MachineOperand::CreateImm(2).getImm());
EXPECT_EQ(MI->getNumOperands(), 4U);

MachineOperand Ops[] = {
MachineOperand::CreateImm(42), MachineOperand::CreateImm(1024),
MachineOperand::CreateImm(2048), MachineOperand::CreateImm(4096),
MachineOperand::CreateImm(8192),
};
auto *It = MI->operands_begin();
++It;
MI->insert(It, Ops);

EXPECT_EQ(MI->getNumOperands(), 9U);
EXPECT_EQ(MI->getOperand(0).getImm(), MachineOperand::CreateImm(0).getImm());
EXPECT_EQ(MI->getOperand(1).getImm(), MachineOperand::CreateImm(42).getImm());
EXPECT_EQ(MI->getOperand(2).getImm(),
MachineOperand::CreateImm(1024).getImm());
EXPECT_EQ(MI->getOperand(3).getImm(),
MachineOperand::CreateImm(2048).getImm());
EXPECT_EQ(MI->getOperand(4).getImm(),
MachineOperand::CreateImm(4096).getImm());
EXPECT_EQ(MI->getOperand(5).getImm(),
MachineOperand::CreateImm(8192).getImm());
EXPECT_EQ(MI->getOperand(6).getImm(), MachineOperand::CreateImm(2).getImm());
EXPECT_EQ(MI->getOperand(7).getImm(), MachineOperand::CreateImm(3).getImm());
EXPECT_EQ(MI->getOperand(8).getImm(), MachineOperand::CreateImm(4).getImm());

// test tied operands
MCRegisterClass MRC{0, 0, 0, 0, 0, 0, 0, 0, /*Allocatable=*/true};
TargetRegisterClass RC{&MRC, 0, 0, {}, 0, 0, 0, 0, 0, 0, 0};
// MachineRegisterInfo will be very upset if these registers aren't
// allocatable.
assert(RC.isAllocatable() && "unusable TargetRegisterClass");
MachineRegisterInfo &MRI = MF->getRegInfo();
Register A = MRI.createVirtualRegister(&RC);
Register B = MRI.createVirtualRegister(&RC);
MI->getOperand(0).ChangeToRegister(A, /*isDef=*/true);
MI->getOperand(1).ChangeToRegister(B, /*isDef=*/false);
MI->tieOperands(0, 1);
EXPECT_TRUE(MI->getOperand(0).isTied());
EXPECT_TRUE(MI->getOperand(1).isTied());
EXPECT_EQ(MI->findTiedOperandIdx(0), 1U);
EXPECT_EQ(MI->findTiedOperandIdx(1), 0U);
MI->insert(&MI->getOperand(1), {MachineOperand::CreateImm(7)});
EXPECT_TRUE(MI->getOperand(0).isTied());
EXPECT_TRUE(MI->getOperand(1).isImm());
EXPECT_TRUE(MI->getOperand(2).isTied());
EXPECT_EQ(MI->findTiedOperandIdx(0), 2U);
EXPECT_EQ(MI->findTiedOperandIdx(2), 0U);
EXPECT_EQ(MI->getOperand(0).getReg(), A);
EXPECT_EQ(MI->getOperand(2).getReg(), B);

// bad inputs
EXPECT_EQ(MI->getNumOperands(), 10U);
MI->insert(MI->operands_begin(), {});
EXPECT_EQ(MI->getNumOperands(), 10U);
}

} // end namespace