CodeGen: Record tied virtual register operands in finalizeBundle

[nhaehnle]

This is in preparation of a future AMDGPU change where we are going to
create bundles before register allocation and want to rely on the
TwoAddressInstructionPass handling those bundles correctly.

v2:

• simplify the virtual register check and the test

---

Stack:

• [5/5] CodeGen/AMDGPU: Allow 3-address conversion of bundled instructions #166213
• [4/5] CodeGen: Handle bundled instructions in two-address-instructions pass #166212
• [3/5] CodeGen: More accurate mayAlias for instructions with multiple MMOs #166211
• [2/5] CodeGen: Record MMOs in finalizeBundle #166210
• [1/5] CodeGen: Record tied virtual register operands in finalizeBundle #166209 ⬅

⚠️ Part of a stack created by spr. Merging this PR using the GitHub UI may have unexpected results.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Nicolai Hähnle (nhaehnle)

Changes

This is in preparation of a future AMDGPU change where we are going to
create bundles before register allocation and want to rely on the
TwoAddressInstructionPass handling those bundles correctly.

---

Stack:

• [5/5] #166213
• [4/5] #166212
• [3/5] #166211
• [2/5] #166210
• [1/5] #166209 ⬅

⚠️ Part of a stack created by spr. Merging this PR using the GitHub UI may have unexpected results.

---

Full diff: https://github.com/llvm/llvm-project/pull/166209.diff

3 Files Affected:

• (modified) llvm/lib/CodeGen/MachineInstrBundle.cpp (+22-1)
• (modified) llvm/unittests/Target/AMDGPU/CMakeLists.txt (+1)
• (added) llvm/unittests/Target/AMDGPU/MachineInstrBundleTest.cpp (+61)

diff --git a/llvm/lib/CodeGen/MachineInstrBundle.cpp b/llvm/lib/CodeGen/MachineInstrBundle.cpp index da29ffc9d2fed..1f47dc50351d2 100644 --- a/llvm/lib/CodeGen/MachineInstrBundle.cpp +++ b/llvm/lib/CodeGen/MachineInstrBundle.cpp @@ -136,6 +136,7 @@ void llvm::finalizeBundle(MachineBasicBlock &MBB, SmallSetVector<Register, 8> ExternUses; SmallSet<Register, 8> KilledUseSet; SmallSet<Register, 8> UndefUseSet; + SmallVector<std::pair<Register, Register>> TiedOperands; for (auto MII = FirstMI; MII != LastMI; ++MII) { // Debug instructions have no effects to track. if (MII->isDebugInstr()) @@ -161,6 +162,16 @@ void llvm::finalizeBundle(MachineBasicBlock &MBB, // External def is now killed. KilledUseSet.insert(Reg); } + if (MO.isTied()) { + // Record tied operand constraints that involve virtual registers so + // that bundles that are formed pre-register allocation reflect the + // relevant constraints. + unsigned TiedIdx = MII->findTiedOperandIdx(MO.getOperandNo()); + MachineOperand &TiedMO = MII->getOperand(TiedIdx); + Register DefReg = TiedMO.getReg(); + if (Reg.isVirtual() || DefReg.isVirtual()) + TiedOperands.emplace_back(DefReg, Reg); + } } } @@ -203,7 +214,17 @@ void llvm::finalizeBundle(MachineBasicBlock &MBB, bool isKill = KilledUseSet.contains(Reg); bool isUndef = UndefUseSet.contains(Reg); MIB.addReg(Reg, getKillRegState(isKill) | getUndefRegState(isUndef) | - getImplRegState(true)); + getImplRegState(true)); + } + + for (auto [DefReg, UseReg] : TiedOperands) { + unsigned DefIdx = + std::distance(LocalDefs.begin(), llvm::find(LocalDefs, DefReg)); + unsigned UseIdx = + std::distance(ExternUses.begin(), llvm::find(ExternUses, UseReg)); + assert(DefIdx < LocalDefs.size()); + assert(UseIdx < ExternUses.size()); + MIB->tieOperands(DefIdx, LocalDefs.size() + UseIdx); } } diff --git a/llvm/unittests/Target/AMDGPU/CMakeLists.txt b/llvm/unittests/Target/AMDGPU/CMakeLists.txt index d6cbaf3f3fb5d..e3748bcea9422 100644 --- a/llvm/unittests/Target/AMDGPU/CMakeLists.txt +++ b/llvm/unittests/Target/AMDGPU/CMakeLists.txt @@ -24,5 +24,6 @@ add_llvm_target_unittest(AMDGPUTests DwarfRegMappings.cpp ExecMayBeModifiedBeforeAnyUse.cpp LiveRegUnits.cpp + MachineInstrBundleTest.cpp PALMetadata.cpp ) diff --git a/llvm/unittests/Target/AMDGPU/MachineInstrBundleTest.cpp b/llvm/unittests/Target/AMDGPU/MachineInstrBundleTest.cpp new file mode 100644 index 0000000000000..3ec4875958e73 --- /dev/null +++ b/llvm/unittests/Target/AMDGPU/MachineInstrBundleTest.cpp @@ -0,0 +1,61 @@ +//===- MachineInstrBundleTest.cpp -----------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "AMDGPUTargetMachine.h" +#include "AMDGPUUnitTests.h" +#include "llvm/CodeGen/MachineFunction.h" +#include "llvm/CodeGen/MachineInstrBuilder.h" +#include "llvm/CodeGen/MachineModuleInfo.h" +#include "gtest/gtest.h" + +using namespace llvm; + +namespace { + +TEST(AMDGPU, TiedOperandsBundle) { + auto TM = createAMDGPUTargetMachine("amdgcn-amd-", "gfx1100", ""); + + GCNSubtarget ST(TM->getTargetTriple(), std::string(TM->getTargetCPU()), + std::string(TM->getTargetFeatureString()), *TM); + + LLVMContext Ctx; + Module Mod("Module", Ctx); + Mod.setDataLayout(TM->createDataLayout()); + + auto *Type = FunctionType::get(Type::getVoidTy(Ctx), false); + auto *F = Function::Create(Type, GlobalValue::ExternalLinkage, "Test", &Mod); + + MachineModuleInfo MMI(TM.get()); + auto MF = + std::make_unique<MachineFunction>(*F, *TM, ST, MMI.getContext(), 42); + auto *BB = MF->CreateMachineBasicBlock(); + MF->push_back(BB); + + const auto &TII = *ST.getInstrInfo(); + auto &MRI = MF->getRegInfo(); + + Register RX = MRI.createVirtualRegister(&AMDGPU::SReg_32_XEXEC_HIRegClass); + Register RY = MRI.createVirtualRegister(&AMDGPU::SReg_32_XEXEC_HIRegClass); + + MachineInstr *MI = + BuildMI(*BB, BB->begin(), {}, TII.get(AMDGPU::COPY), RX).addReg(RY); + MI->tieOperands(0, 1); + + finalizeBundle(*BB, MI->getIterator(), std::next(MI->getIterator())); + + MachineInstr *Bundle = &*BB->begin(); + EXPECT_TRUE(Bundle->getOpcode() == AMDGPU::BUNDLE); + EXPECT_EQ(Bundle->getNumOperands(), 2u); + EXPECT_EQ(Bundle->getOperand(0).getReg(), RX); + EXPECT_EQ(Bundle->getOperand(1).getReg(), RY); + EXPECT_TRUE(Bundle->getOperand(0).isTied()); + EXPECT_TRUE(Bundle->getOperand(1).isTied()); + EXPECT_EQ(Bundle->findTiedOperandIdx(0), 1u); +} + +} // end namespace 

[arsenm]

I think checking if one or the other is virtual should be sufficient

[jayfoad]

See test/CodeGen/AMDGPU/finalizebundle.mir for a much nicer lit-based way to test finalizeBundle.

[nhaehnle]

Indeed, that is a lot nicer