Skip to content

[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.

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

Merged
merged 6 commits into from
Oct 6, 2025
Merged

[AMDGPU][MC] GFX9 - allow op_sel in v_interp_p2_f16 #150712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56be09f
[AMDGPU][MC] GFX9 - allow op_sel in v_interp_p2_f16
jwanggit86 Jul 25, 2025
64b8878
This commit:
jwanggit86 Aug 14, 2025
751178a
Fix code format
jwanggit86 Aug 14, 2025
6aaf38e
Create a helper method for common code in two functions
jwanggit86 Sep 30, 2025
01fa59a
Printer should print 4 bits for op_sel
jwanggit86 Oct 2, 2025
688142d
Fix code formatting
jwanggit86 Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 40 additions & 23 deletions llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

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?

Copy link
Contributor Author

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).


bool parseDimId(unsigned &Encoding);
ParseStatus parseDim(OperandVector &Operands);
Expand Down Expand Up @@ -9177,6 +9178,33 @@ static bool isRegOrImmWithInputMods(const MCInstrDesc &Desc, unsigned OpNum) {
MCOI::OperandConstraint::TIED_TO) == -1;
}

void AMDGPUAsmParser::cvtOpSelHelper(MCInst &Inst, unsigned OpSel) {
unsigned Opc = Inst.getOpcode();
constexpr AMDGPU::OpName Ops[] = {AMDGPU::OpName::src0, AMDGPU::OpName::src1,
AMDGPU::OpName::src2};
constexpr AMDGPU::OpName ModOps[] = {AMDGPU::OpName::src0_modifiers,
AMDGPU::OpName::src1_modifiers,
AMDGPU::OpName::src2_modifiers};
for (int J = 0; J < 3; ++J) {
int OpIdx = AMDGPU::getNamedOperandIdx(Opc, Ops[J]);
if (OpIdx == -1)
// Some instructions, e.g. v_interp_p2_f16 in GFX9, have src0, src2, but
// no src1. So continue instead of break.
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;
Expand Down Expand Up @@ -9213,6 +9241,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)
Expand Down Expand Up @@ -9248,31 +9286,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;
Expand Down
11 changes: 11 additions & 0 deletions llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1280,6 +1280,17 @@ void AMDGPUInstPrinter::printPackedModifier(const MCInst *MI,
(ModIdx != -1) ? MI->getOperand(ModIdx).getImm() : DefaultValue;
}

// Some instructions, e.g. v_interp_p2_f16 in GFX9, have src0, src2, but no
// src1.
if (NumOps == 1 && AMDGPU::hasNamedOperand(Opc, AMDGPU::OpName::src2) &&
!AMDGPU::hasNamedOperand(Opc, AMDGPU::OpName::src1)) {
Ops[NumOps++] = DefaultValue; // Set src1_modifiers to default.
int Mod2Idx =
AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src2_modifiers);
assert(Mod2Idx != -1);
Ops[NumOps++] = MI->getOperand(Mod2Idx).getImm();
}

const bool HasDst =
(AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst) != -1) ||
(AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::sdst) != -1);
Expand Down
31 changes: 22 additions & 9 deletions llvm/lib/Target/AMDGPU/VOP3Instructions.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ class VOP3Interp<string OpName, VOPProfile P, list<dag> pattern = []> :
VOP3_Pseudo<OpName, P, pattern> {
let AsmMatchConverter = "cvtVOP3Interp";
let mayRaiseFPException = 0;
let VOP3_OPSEL = P.HasOpSel;
}

def VOP3_INTERP : VOPProfile<[f32, f32, i32, untyped]> {
Expand All @@ -119,16 +120,17 @@ def VOP3_INTERP_MOV : VOPProfile<[f32, i32, i32, untyped]> {
let HasSrc0Mods = 0;
}

class getInterp16Asm <bit HasSrc2, bit HasOMod> {
class getInterp16Asm <bit HasSrc2, bit HasOMod, bit OpSel> {
string src2 = !if(HasSrc2, ", $src2_modifiers", "");
string omod = !if(HasOMod, "$omod", "");
string opsel = !if(OpSel, "$op_sel", "");
string ret =
" $vdst, $src0_modifiers, $attr$attrchan"#src2#"$high$clamp"#omod;
" $vdst, $src0_modifiers, $attr$attrchan"#src2#"$high$clamp"#omod#opsel;
}

class getInterp16Ins <bit HasSrc2, bit HasOMod,
Operand Src0Mod, Operand Src2Mod> {
dag ret = !if(HasSrc2,
Operand Src0Mod, Operand Src2Mod, bit OpSel> {
dag ret1 = !if(HasSrc2,
!if(HasOMod,
(ins Src0Mod:$src0_modifiers, VRegSrc_32:$src0,
InterpAttr:$attr, InterpAttrChan:$attrchan,
Expand All @@ -143,19 +145,22 @@ class getInterp16Ins <bit HasSrc2, bit HasOMod,
InterpAttr:$attr, InterpAttrChan:$attrchan,
highmod:$high, Clamp0:$clamp, omod0:$omod)
);
dag ret2 = !if(OpSel, (ins op_sel0:$op_sel), (ins));
dag ret = !con(ret1, ret2);
}

class VOP3_INTERP16 <list<ValueType> ArgVT> : VOPProfile<ArgVT> {
class VOP3_INTERP16 <list<ValueType> ArgVT, bit OpSel = 0> : VOPProfile<ArgVT> {
let IsSingle = 1;
let HasOMod = !ne(DstVT.Value, f16.Value);
let HasHigh = 1;
let HasOpSel = OpSel;

let Src0Mod = FPVRegInputMods;
let Src2Mod = FPVRegInputMods;

let Outs64 = (outs DstRC.RegClass:$vdst);
let Ins64 = getInterp16Ins<HasSrc2, HasOMod, Src0Mod, Src2Mod>.ret;
let Asm64 = getInterp16Asm<HasSrc2, HasOMod>.ret;
let Ins64 = getInterp16Ins<HasSrc2, HasOMod, Src0Mod, Src2Mod, OpSel>.ret;
let Asm64 = getInterp16Asm<HasSrc2, HasOMod, OpSel>.ret;
}

//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -481,7 +486,7 @@ let SubtargetPredicate = isGFX9Plus in {
defm V_MAD_U16_gfx9 : VOP3Inst_t16 <"v_mad_u16_gfx9", VOP_I16_I16_I16_I16>;
defm V_MAD_I16_gfx9 : VOP3Inst_t16 <"v_mad_i16_gfx9", VOP_I16_I16_I16_I16>;
let OtherPredicates = [isNotGFX90APlus] in
def V_INTERP_P2_F16_gfx9 : VOP3Interp <"v_interp_p2_f16_gfx9", VOP3_INTERP16<[f16, f32, i32, f32]>>;
def V_INTERP_P2_F16_opsel : VOP3Interp <"v_interp_p2_f16_opsel", VOP3_INTERP16<[f16, f32, i32, f32], /*OpSel*/ 1>>;
} // End SubtargetPredicate = isGFX9Plus

// This predicate should only apply to the selection pattern. The
Expand Down Expand Up @@ -2678,6 +2683,14 @@ multiclass VOP3Interp_F16_Real_gfx9<bits<10> op, string OpName, string AsmName>
}
}

multiclass VOP3Interp_F16_OpSel_Real_gfx9<bits<10> op, string OpName, string AsmName> {
def _gfx9 : VOP3_Real<!cast<VOP3_Pseudo>(OpName), SIEncodingFamily.GFX9>,
VOP3Interp_OpSel_gfx9 <op, !cast<VOP3_Pseudo>(OpName).Pfl> {
VOP3_Pseudo ps = !cast<VOP3_Pseudo>(OpName);
let AsmString = AsmName # ps.AsmOperands;
}
}

multiclass VOP3_Real_gfx9<bits<10> op, string AsmName> {
def _gfx9 : VOP3_Real<!cast<VOP_Pseudo>(NAME#"_e64"), SIEncodingFamily.GFX9>,
VOP3e_vi <op, !cast<VOP_Pseudo>(NAME#"_e64").Pfl> {
Expand Down Expand Up @@ -2790,7 +2803,7 @@ defm V_MAD_U16_gfx9 : VOP3OpSel_F16_Real_gfx9 <0x204, "v_mad_u16">;
defm V_MAD_I16_gfx9 : VOP3OpSel_F16_Real_gfx9 <0x205, "v_mad_i16">;
defm V_FMA_F16_gfx9 : VOP3OpSel_F16_Real_gfx9 <0x206, "v_fma_f16">;
defm V_DIV_FIXUP_F16_gfx9 : VOP3OpSel_F16_Real_gfx9 <0x207, "v_div_fixup_f16">;
defm V_INTERP_P2_F16_gfx9 : VOP3Interp_F16_Real_gfx9 <0x277, "V_INTERP_P2_F16_gfx9", "v_interp_p2_f16">;
defm V_INTERP_P2_F16_opsel : VOP3Interp_F16_OpSel_Real_gfx9 <0x277, "V_INTERP_P2_F16_opsel", "v_interp_p2_f16">;

defm V_ADD_I32 : VOP3_Real_vi <0x29c>;
defm V_SUB_I32 : VOP3_Real_vi <0x29d>;
Expand Down
7 changes: 7 additions & 0 deletions llvm/lib/Target/AMDGPU/VOPInstructions.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,13 @@ class VOP3a_ScaleSel_gfx1250<bits<10> op, VOPProfile p> : VOP3e_gfx11_gfx12<op,
let Inst{14-11} = scale_sel;
}

class VOP3Interp_OpSel_gfx9<bits<10> op, VOPProfile p> : VOP3Interp_vi<op, p> {
let Inst{11} = src0_modifiers{2};
// There's no src1
let Inst{13} = src2_modifiers{2};
let Inst{14} = !if(p.HasDst, src0_modifiers{3}, 0);
}

class VOP3Interp_gfx10<bits<10> op, VOPProfile p> : VOP3e_gfx10<op, p> {
bits<6> attr;
bits<2> attrchan;
Expand Down
135 changes: 135 additions & 0 deletions llvm/test/MC/AMDGPU/vop3-gfx9.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,141 @@ 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]
// 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,1]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,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,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,1]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,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:[1,0,0]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,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,1]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,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:[0,1,1]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,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:[1,0,0]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,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,1]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,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,0]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,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,1]
// GFX9: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,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:[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,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,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,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,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,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,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,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,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,0,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]
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

$ cat z.s
v_pk_max_f16 v5, v1, v2 op_sel:[1,1,1]
v_pk_max_f16 v5, v1, v2 op_sel:[1,1,1,1]

$ ../../build/bin/llvm-mc -triple=amdgcn -mcpu=gfx908 -show-encoding z.s
        v_pk_max_f16 v5, v1, v2 op_sel:[1,1]    ; encoding: [0x05,0x58,0x92,0xd3,0x01,0x05,0x02,0x18]
        v_pk_max_f16 v5, v1, v2 op_sel:[1,1]    ; encoding: [0x05,0x58,0x92,0xd3,0x01,0x05,0x02,0x18]

$ cat z.txt
0x05,0x58,0x92,0xd3,0x01,0x05,0x02,0x18

$ ../../build/bin/llvm-mc -triple=amdgcn -mcpu=gfx908 -show-encoding -disassemble z.txt
        v_pk_max_f16 v5, v1, v2 op_sel:[1,1]    ; encoding: [0x05,0x58,0x92,0xd3,0x01,0x05,0x02,0x18]

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it hard to read that v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,1,0] &
v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,1,0] represent the same bits. For the second representation you have to know src1 does not take an opsel so the idx 1 value in the op_sel array is for src2. I don't have a huge issue if the value of junk bits change in a round trip either, but this is about which indices in the op_sel map to which source operands. I don't like the idea of doing lots of extra work to validate the number of op_sel bits either. I won't block the review over this, just pointing out it could lead to confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,0,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,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,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,0,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,0,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
Expand Down
21 changes: 21 additions & 0 deletions llvm/test/MC/Disassembler/AMDGPU/gfx9_vop3.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19311,6 +19311,27 @@
# CHECK: v_interp_p2_f16 v5, v2, attr0.x, v3 clamp ; encoding: [0x05,0x80,0x77,0xd2,0x00,0x04,0x0e,0x04]
0x05,0x80,0x77,0xd2,0x00,0x04,0x0e,0x04

# CHECK: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,0,1] ; encoding: [0x05,0x40,0x77,0xd2,0x00,0x04,0x0e,0x04]
0x05,0x40,0x77,0xd2,0x00,0x04,0x0e,0x04

# CHECK: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,1,0] ; encoding: [0x05,0x20,0x77,0xd2,0x00,0x04,0x0e,0x04]
0x05,0x20,0x77,0xd2,0x00,0x04,0x0e,0x04

# CHECK: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[0,0,1,1] ; encoding: [0x05,0x60,0x77,0xd2,0x00,0x04,0x0e,0x04]
0x05,0x60,0x77,0xd2,0x00,0x04,0x0e,0x04

# CHECK: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,0,0] ; encoding: [0x05,0x08,0x77,0xd2,0x00,0x04,0x0e,0x04]
0x05,0x08,0x77,0xd2,0x00,0x04,0x0e,0x04

# CHECK: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,0,1] ; encoding: [0x05,0x48,0x77,0xd2,0x00,0x04,0x0e,0x04]
0x05,0x48,0x77,0xd2,0x00,0x04,0x0e,0x04

# CHECK: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,1,0] ; encoding: [0x05,0x28,0x77,0xd2,0x00,0x04,0x0e,0x04]
0x05,0x28,0x77,0xd2,0x00,0x04,0x0e,0x04

# CHECK: v_interp_p2_f16 v5, v2, attr0.x, v3 op_sel:[1,0,1,1] ; encoding: [0x05,0x68,0x77,0xd2,0x00,0x04,0x0e,0x04]
0x05,0x68,0x77,0xd2,0x00,0x04,0x0e,0x04

# CHECK: v_add_f64 v[5:6], v[1:2], v[2:3] ; encoding: [0x05,0x00,0x80,0xd2,0x01,0x05,0x02,0x00]
0x05,0x00,0x80,0xd2,0x01,0x05,0x02,0x00

Expand Down