Skip to content

[AMDGPU][MC] Improve error message for missing dim operand #96588

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 3 commits into from
Jul 23, 2024
Merged

[AMDGPU][MC] Improve error message for missing dim operand #96588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e77637
[AMDGPU][MC] Improve error message for missing dim operand
Jun 25, 2024
ee60450
Replace hard-coded switch on image_bvh opcodes.
Jul 9, 2024
8a2e5e6
Update comment for dim definition.
Jul 22, 2024
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
51 changes: 51 additions & 0 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 @@ -1739,6 +1739,7 @@ class AMDGPUAsmParser : public MCTargetAsmParser {
bool validateMIMGDataSize(const MCInst &Inst, const SMLoc &IDLoc);
bool validateMIMGAddrSize(const MCInst &Inst, const SMLoc &IDLoc);
bool validateMIMGD16(const MCInst &Inst);
bool validateMIMGDim(const MCInst &Inst, const OperandVector &Operands);
bool validateMIMGMSAA(const MCInst &Inst);
bool validateOpSel(const MCInst &Inst);
bool validateNeg(const MCInst &Inst, int OpName);
Expand Down Expand Up @@ -4006,6 +4007,52 @@ bool AMDGPUAsmParser::validateMIMGGatherDMask(const MCInst &Inst) {
return DMask == 0x1 || DMask == 0x2 || DMask == 0x4 || DMask == 0x8;
}

bool AMDGPUAsmParser::validateMIMGDim(const MCInst &Inst,
const OperandVector &Operands) {
const unsigned Opc = Inst.getOpcode();
const MCInstrDesc &Desc = MII.get(Opc);

if ((Desc.TSFlags & MIMGFlags) == 0)
return true;

if (!isGFX10Plus())
return true;

switch (Opc) {
case IMAGE_BVH64_INTERSECT_RAY_a16_gfx12:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid this hardcoded switch of specific opcodes?

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 seems this can be done by checking the boolean AMDGPU::getMIMGBaseOpcode(Opc)->BVH. See latest commit.

case IMAGE_BVH64_INTERSECT_RAY_a16_nsa_gfx10:
case IMAGE_BVH64_INTERSECT_RAY_a16_nsa_gfx11:
case IMAGE_BVH64_INTERSECT_RAY_a16_sa_gfx10:
case IMAGE_BVH64_INTERSECT_RAY_a16_sa_gfx11:
case IMAGE_BVH64_INTERSECT_RAY_gfx12:
case IMAGE_BVH64_INTERSECT_RAY_nsa_gfx10:
case IMAGE_BVH64_INTERSECT_RAY_nsa_gfx11:
case IMAGE_BVH64_INTERSECT_RAY_sa_gfx10:
case IMAGE_BVH64_INTERSECT_RAY_sa_gfx11:

case IMAGE_BVH_INTERSECT_RAY_a16_gfx12:
case IMAGE_BVH_INTERSECT_RAY_a16_nsa_gfx10:
case IMAGE_BVH_INTERSECT_RAY_a16_nsa_gfx11:
case IMAGE_BVH_INTERSECT_RAY_a16_sa_gfx10:
case IMAGE_BVH_INTERSECT_RAY_a16_sa_gfx11:
case IMAGE_BVH_INTERSECT_RAY_gfx12:
case IMAGE_BVH_INTERSECT_RAY_nsa_gfx10:
case IMAGE_BVH_INTERSECT_RAY_nsa_gfx11:
case IMAGE_BVH_INTERSECT_RAY_sa_gfx10:
case IMAGE_BVH_INTERSECT_RAY_sa_gfx11:
return true;
default:
break;
}

for (unsigned i = 1, e = Operands.size(); i != e; ++i) {
AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[i]);
if (Op.isDim())
return true;
}
return false;
}

bool AMDGPUAsmParser::validateMIMGMSAA(const MCInst &Inst) {
const unsigned Opc = Inst.getOpcode();
const MCInstrDesc &Desc = MII.get(Opc);
Expand Down Expand Up @@ -5094,6 +5141,10 @@ bool AMDGPUAsmParser::validateInstruction(const MCInst &Inst,
"d16 modifier is not supported on this GPU");
return false;
}
if (!validateMIMGDim(Inst, Operands)) {
Error(IDLoc, "missing dim operand");
return false;
}
if (!validateMIMGMSAA(Inst)) {
Error(getImmLoc(AMDGPUOperand::ImmTyDim, Operands),
"invalid dim; must be MSAA type");
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AMDGPU/SIInstrInfo.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,7 +1089,7 @@ def exp_vm : NamedBitOperand<"vm", "ExpVM">;
def FORMAT : CustomOperand<i8>;

def DMask : NamedIntOperand<i16, "dmask">;
def Dim : CustomOperand<i8>;
def Dim : CustomOperand<i8, 1>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you comment the parameter name here? I have to look to see what this does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean like /OperandName=/ not exposition before the definition

Copy link
Contributor

Choose a reason for hiding this comment

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

also can call it true instead of 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment as suggested.


def dst_sel : SDWAOperand<"dst_sel", "SDWADstSel">;
def src0_sel : SDWAOperand<"src0_sel", "SDWASrc0Sel">;
Expand Down
4 changes: 4 additions & 0 deletions llvm/test/MC/AMDGPU/gfx1030_err.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,7 @@ image_bvh_intersect_ray v[4:7], v[9:16], s[4:7] noa16

image_bvh_intersect_ray v[39:42], [v50, v46, v23, v17, v16, v15, v21, v20], s[12:15] noa16
// GFX10: :[[@LINE-1]]:{{[0-9]+}}: error: image address size does not match a16

// missing dim
image_msaa_load v[1:4], v[5:7], s[8:15] dmask:0xf glc
// GFX10: :[[@LINE-1]]:{{[0-9]+}}: error: missing dim operand
Loading
Loading