Implicit Exec Mask Operand Missing From MCInstrDesc of Some Opcodes in AMDGPU backend

[matinraayai]

The following Opcodes do not have AMDGPU::EXEC mask as an implicit operand in their MC Instr Desc:
GLOBAL_STORE_DWORD_vi
BUFFER_LOAD_DWORD_OFFEN_vi
GLOBAL_STORE_DWORD_SADDR_vi
The issue appears when constructing a llvm::MachineInstr from a valid llvm::MCInst, in the same manner as llvm-exegesis here:

llvm-project/llvm/tools/llvm-exegesis/lib/Assembler.cpp

Lines 157 to 176 in 859de94

	MachineInstrBuilder Builder = BuildMI(MBB, DL, MCID);
	for (unsigned OpIndex = 0, E = Inst.getNumOperands(); OpIndex < E;
	++OpIndex) {
	const MCOperand &Op = Inst.getOperand(OpIndex);
	if (Op.isReg()) {
	const bool IsDef = OpIndex < MCID.getNumDefs();
	unsigned Flags = 0;
	const MCOperandInfo &OpInfo = MCID.operands().begin()[OpIndex];
	if (IsDef && !OpInfo.isOptionalDef())
	Flags |= RegState::Define;
	Builder.addReg(Op.getReg(), Flags);
	} else if (Op.isImm()) {
	Builder.addImm(Op.getImm());
	} else if (!Op.isValid()) {
	llvm_unreachable("Operand is not set");
	} else {
	llvm_unreachable("Not yet implemented");
	}
	}
	}

For the MIR to be correct, simply adding the explicit operands should be enough, as the implicit operands are automatically added according to the MCInstrDesc when calling the llvm::BuildMI here:

llvm-project/llvm/tools/llvm-exegesis/lib/Assembler.cpp

Line 157 in 859de94

MachineInstrBuilder Builder = BuildMI(MBB, DL, MCID);

This method will call the llvm::MachineInstr constructor with NoImplicit flag set to false:

llvm-project/llvm/lib/CodeGen/MachineInstr.cpp

Lines 98 to 114 in 859de94

	MachineInstr::MachineInstr(MachineFunction &MF, const MCInstrDesc &TID,
	DebugLoc DL, bool NoImp)
	: MCID(&TID), NumOperands(0), Flags(0), AsmPrinterFlags(0),
	DbgLoc(std::move(DL)), DebugInstrNum(0) {
	assert(DbgLoc.hasTrivialDestructor() && "Expected trivial destructor");
	// Reserve space for the expected number of operands.
	if (unsigned NumOps = MCID->getNumOperands() + MCID->implicit_defs().size() +
	MCID->implicit_uses().size()) {
	CapOperands = OperandCapacity::get(NumOps);
	Operands = MF.allocateOperandArray(CapOperands);
	}
	if (!NoImp)
	addImplicitDefUseOperands(MF);
	}

However, when printing the constructed MIR, the implicit exec is nowhere to be seen:

GLOBAL_STORE_DWORD_vi $vgpr0_vgpr1, $vgpr2, 0, 0
BUFFER_LOAD_DWORD_OFFEN_vi $vgpr3, $sgpr0_sgpr1_sgpr2_sgpr3, 0, 60, 0, 0
BUFFER_STORE_DWORD_OFFEN_vi $vgpr1, $vgpr3, $sgpr0_sgpr1_sgpr2_sgpr3, 0, 84, 0, 0

This causes issues when these instructions get verified before running CodeGen passes.

CC: @arsenm @kzhuravl

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

Author: Matin Raayai (matinraayai)

The following Opcodes do not have `AMDGPU::EXEC` mask as an implicit operand in their MC Instr Desc: `GLOBAL_STORE_DWORD_vi` `BUFFER_LOAD_DWORD_OFFEN_vi` `GLOBAL_STORE_DWORD_SADDR_vi` The issue appears when constructing a `llvm::MachineInstr` from a valid `llvm::MCInst`, in the same manner as llvm-exegesis here: https://github.com/llvm/llvm-project/blob/859de94536425376244940e190e069a09d797737/llvm/tools/llvm-exegesis/lib/Assembler.cpp#L157-L176 For the MIR to be correct, simply adding the explicit operands should be enough, as the implicit operands are automatically added according to the MCInstrDesc when calling the `llvm::BuildMI` here: https://github.com/llvm/llvm-project/blob/859de94536425376244940e190e069a09d797737/llvm/tools/llvm-exegesis/lib/Assembler.cpp#L157 This method will call the `llvm::MachineInstr` constructor with `NoImplicit` flag set to `false`:

llvm-project/llvm/lib/CodeGen/MachineInstr.cpp

Lines 98 to 114 in 859de94

	MachineInstr::MachineInstr(MachineFunction &MF, const MCInstrDesc &TID,
	DebugLoc DL, bool NoImp)
	: MCID(&TID), NumOperands(0), Flags(0), AsmPrinterFlags(0),
	DbgLoc(std::move(DL)), DebugInstrNum(0) {
	assert(DbgLoc.hasTrivialDestructor() && "Expected trivial destructor");
	// Reserve space for the expected number of operands.
	if (unsigned NumOps = MCID->getNumOperands() + MCID->implicit_defs().size() +
	MCID->implicit_uses().size()) {
	CapOperands = OperandCapacity::get(NumOps);
	Operands = MF.allocateOperandArray(CapOperands);
	}
	if (!NoImp)
	addImplicitDefUseOperands(MF);
	}

However, when printing the constructed MIR, the implicit exec is nowhere to be seen:

GLOBAL_STORE_DWORD_vi $vgpr0_vgpr1, $vgpr2, 0, 0
BUFFER_LOAD_DWORD_OFFEN_vi $vgpr3, $sgpr0_sgpr1_sgpr2_sgpr3, 0, 60, 0, 0
BUFFER_STORE_DWORD_OFFEN_vi $vgpr1, $vgpr3, $sgpr0_sgpr1_sgpr2_sgpr3, 0, 84, 0, 0

This causes issues when these instructions get verified before running CodeGen passes.

CC: @arsenm @kzhuravl

[jayfoad]

This causes issues when these instructions get verified before running CodeGen passes.

Why are you trying to run CodeGen passes and MachineVerifier on Real instructions like GLOBAL_STORE_DWORD_vi?

In normal CodeGen, all the passes run on Pseudo instructions like GLOBAL_STORE_DWORD, which does have the implicit use of EXEC. These Pseudo instructions are only converted to the corresponding Real instruction for the appropriate subtarget as part of final code emission.

[matinraayai]

@jayfoad the main reason is instrumentation. We use MachinePasses to leverage things like the AsmPrinter and pseudo instructions to make our job easier.
Now I can, as you said, somehow convert these opcode to their pseudo equivalent; However:

1. I know there's a way to convert pseudo to MC, I don't know if there's a way to go other way around.
2. I want to keep the lowered MC operands. There's no point adding an extra conversion step here.
3. I've encountered other real instructions that have the EXEC mask as an implicit operand. One example is V_MOV_B32_e32_vi. This seems inconsistent with only pseudo instructions having the implicit operands modeled.

IMO even if the normal CodeGen won't encounter real opcodes, that doesn't mean MC shouldn't model the implicit operands correctly. This is especially true if someone is using MC to analyze a binary; They shouldn't need to convert the opcode to its pseudo equivalent first just to get the correctly modeled behavior.

[jayfoad]

OK. I certainly have no objection to fixing the implicit uses/defs on Real instructions to match the corresponding Pseudo.

[arsenm]

Do the real instructions just not copy the Uses and Defs from the parent pseudo?

[ritter-x2a]

Do the real instructions just not copy the Uses and Defs from the parent pseudo?

Currently, that's not the case for many instructions. This PR changes that: #93004

@matinraayai : feel free to try the commit and check if it solves your problem.

[ritter-x2a]

Closing this, as the merged PR #93004 should fix the issue.