-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[AMDGPU][MC] GFX9 - allow op_sel in v_interp_p2_f16 #150712
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.
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
Changes from 4 commits
56be09f
64b8878
751178a
6aaf38e
01fa59a
688142d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1928,6 +1928,7 @@ class AMDGPUAsmParser : public MCTargetAsmParser { | |
|
||
void cvtVOP3Interp(MCInst &Inst, const OperandVector &Operands); | ||
void cvtVINTERP(MCInst &Inst, const OperandVector &Operands); | ||
void cvtOpSelHelper(MCInst &Inst, unsigned OpSel); | ||
|
||
bool parseDimId(unsigned &Encoding); | ||
ParseStatus parseDim(OperandVector &Operands); | ||
|
@@ -9177,6 +9178,32 @@ static bool isRegOrImmWithInputMods(const MCInstrDesc &Desc, unsigned OpNum) { | |
MCOI::OperandConstraint::TIED_TO) == -1; | ||
} | ||
|
||
void AMDGPUAsmParser::cvtOpSelHelper(MCInst &Inst, unsigned OpSel) { | ||
unsigned Opc = Inst.getOpcode(); | ||
const AMDGPU::OpName Ops[] = {AMDGPU::OpName::src0, AMDGPU::OpName::src1, | ||
AMDGPU::OpName::src2}; | ||
const AMDGPU::OpName ModOps[] = {AMDGPU::OpName::src0_modifiers, | ||
AMDGPU::OpName::src1_modifiers, | ||
AMDGPU::OpName::src2_modifiers}; | ||
|
||
// Some v_interp instructions in GFX9 have src0, src2, but no src1. | ||
for (int J = 0; J < 3; ++J) { | ||
int OpIdx = AMDGPU::getNamedOperandIdx(Opc, Ops[J]); | ||
if (OpIdx == -1) | ||
continue; | ||
|
||
int ModIdx = AMDGPU::getNamedOperandIdx(Opc, ModOps[J]); | ||
uint32_t ModVal = Inst.getOperand(ModIdx).getImm(); | ||
|
||
if ((OpSel & (1 << J)) != 0) | ||
ModVal |= SISrcMods::OP_SEL_0; | ||
// op_sel[3] is encoded in src0_modifiers. | ||
if (ModOps[J] == AMDGPU::OpName::src0_modifiers && (OpSel & (1 << 3)) != 0) | ||
ModVal |= SISrcMods::DST_OP_SEL; | ||
|
||
Inst.getOperand(ModIdx).setImm(ModVal); | ||
} | ||
} | ||
|
||
void AMDGPUAsmParser::cvtVOP3Interp(MCInst &Inst, const OperandVector &Operands) | ||
{ | ||
OptionalImmIndexMap OptionalIdx; | ||
|
@@ -9213,6 +9240,16 @@ void AMDGPUAsmParser::cvtVOP3Interp(MCInst &Inst, const OperandVector &Operands) | |
if (AMDGPU::hasNamedOperand(Opc, AMDGPU::OpName::omod)) | ||
addOptionalImmOperand(Inst, Operands, OptionalIdx, | ||
AMDGPUOperand::ImmTyOModSI); | ||
|
||
// Some v_interp instructions use op_sel[3] for dst. | ||
if (AMDGPU::hasNamedOperand(Opc, AMDGPU::OpName::op_sel)) { | ||
addOptionalImmOperand(Inst, Operands, OptionalIdx, | ||
AMDGPUOperand::ImmTyOpSel); | ||
int OpSelIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::op_sel); | ||
unsigned OpSel = Inst.getOperand(OpSelIdx).getImm(); | ||
|
||
cvtOpSelHelper(Inst, OpSel); | ||
} | ||
} | ||
|
||
void AMDGPUAsmParser::cvtVINTERP(MCInst &Inst, const OperandVector &Operands) | ||
|
@@ -9248,31 +9285,10 @@ void AMDGPUAsmParser::cvtVINTERP(MCInst &Inst, const OperandVector &Operands) | |
if (OpSelIdx == -1) | ||
return; | ||
|
||
const AMDGPU::OpName Ops[] = {AMDGPU::OpName::src0, AMDGPU::OpName::src1, | ||
AMDGPU::OpName::src2}; | ||
const AMDGPU::OpName ModOps[] = {AMDGPU::OpName::src0_modifiers, | ||
AMDGPU::OpName::src1_modifiers, | ||
AMDGPU::OpName::src2_modifiers}; | ||
|
||
unsigned OpSel = Inst.getOperand(OpSelIdx).getImm(); | ||
|
||
for (int J = 0; J < 3; ++J) { | ||
int OpIdx = AMDGPU::getNamedOperandIdx(Opc, Ops[J]); | ||
if (OpIdx == -1) | ||
break; | ||
|
||
int ModIdx = AMDGPU::getNamedOperandIdx(Opc, ModOps[J]); | ||
uint32_t ModVal = Inst.getOperand(ModIdx).getImm(); | ||
|
||
if ((OpSel & (1 << J)) != 0) | ||
ModVal |= SISrcMods::OP_SEL_0; | ||
if (ModOps[J] == AMDGPU::OpName::src0_modifiers && | ||
(OpSel & (1 << 3)) != 0) | ||
ModVal |= SISrcMods::DST_OP_SEL; | ||
|
||
Inst.getOperand(ModIdx).setImm(ModVal); | ||
} | ||
cvtOpSelHelper(Inst, OpSel); | ||
} | ||
|
||
void AMDGPUAsmParser::cvtScaledMFMA(MCInst &Inst, | ||
const OperandVector &Operands) { | ||
OptionalImmIndexMap OptionalIdx; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -566,6 +566,86 @@ v_interp_p2_f16 v5, v2, attr0.x, v3 clamp | |
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// VI: v_interp_p2_f16 v5, v2, attr0.x, v3 clamp ; encoding: [0x05,0x80,0x76,0xd2,0x00,0x04,0x0e,0x04] | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,0,0] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 ; encoding: [0x05,0x00,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,0,1] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,1] ; encoding: [0x05,0x40,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,1,0] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,1,0] ; encoding: [0x05,0x20,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,1,1] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,1,1] ; encoding: [0x05,0x60,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,1,0,0] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 ; encoding: [0x05,0x00,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,1,0,1] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,1] ; encoding: [0x05,0x40,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,1,1,0] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,1,0] ; encoding: [0x05,0x20,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,1,1,1] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,1,1] ; encoding: [0x05,0x60,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,0,0] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,0] ; encoding: [0x05,0x08,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,0,1] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,1] ; encoding: [0x05,0x48,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,1,0] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,0] ; encoding: [0x05,0x28,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,1,1] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that this does not round trip from assembly to disassembly will be pretty confusing for users I think. @rampitec What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I know, round-trip already is not always guaranteed for op_sel, e.g. when the input has extraneous bits. Here is an example:
Here only two bits are expected in op_sel, but the input instructions have more. The extra bits cannot be encoded. So the assembler only encodes the two valid bits, and the printer only prints two bits. The disassembler only sees two bits, and only prints two bits. One option is for the assembler to reject the instruction when there are too many op_sel bits, which is a non-trivial change. Do you think we should do this? @rampitec @Sisyph There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't round trip lots of extraneous constructs, this isn't unique. I don't think it's worth putting extra effort into preserving defaulted junk bits. If they are set to non-default values, I'd almost expect them to pass through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it hard to read that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the given example, the input and output are different because the input is incorrect with 4 bits while the instruction only takes 3. If we don't want to do the extra work of checking the number of op_sel bits, then the extraneous bit is silently ignored, which is what happened in the given example, and, as pointed out above, is also what happens elsewhere with op_sel. |
||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,1] ; encoding: [0x05,0x68,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,0,0] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,0] ; encoding: [0x05,0x08,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,0,1] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,1] ; encoding: [0x05,0x48,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,1,0] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,0] ; encoding: [0x05,0x28,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,1,1] | ||
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,1] ; encoding: [0x05,0x68,0x77,0xd2,0x00,0x04,0x0e,0x04] | ||
// NOSICI: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
// NOVI: :[[@LINE-3]]:{{[0-9]+}}: error: not a valid operand. | ||
|
||
v_interp_p2_legacy_f16 v5, v2, attr31.x, v3 | ||
// GFX9: v_interp_p2_legacy_f16 v5, v2, attr31.x, v3 ; encoding: [0x05,0x00,0x76,0xd2,0x1f,0x04,0x0e,0x04] | ||
// NOGCN: :[[@LINE-2]]:{{[0-9]+}}: error: instruction not supported on this GPU | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me what this helper does. Can you make the function name more clear, and add comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It converts the raw op_sel value in the asm instruction to an MCInst operand. This function was created to avoid code duplication in two functions (the two call sites).