Skip to content
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

Jump to bottom

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

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 2 commits
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
28 changes: 28 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,29 @@ bool AMDGPUAsmParser::validateMIMGGatherDMask(const MCInst &Inst) {
return DMask == 0x1 || DMask == 0x2 || DMask == 0x4 || DMask == 0x8;
}

bool AMDGPUAsmParser::validateMIMGDim(const MCInst &Inst,
const OperandVector &Operands) {
if (!isGFX10Plus())
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be a named feature instead of a generation check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not able to find a named feature for "dim". Note that isGFX10Plus() etc are defined as member functions of the class AMDGPUAsmParser and are used throughout the file. For example, the first few lines of parseDim() include a call of isGFX10Plus():

ParseStatus AMDGPUAsmParser::parseDim(OperandVector &Operands) {
  if (!isGFX10Plus())
    return ParseStatus::NoMatch;
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, you would need to add it. Or at least hide this in an alias that shows why the version matters

return true;

const unsigned Opc = Inst.getOpcode();
const MCInstrDesc &Desc = MII.get(Opc);

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

// image_bvh_intersect_ray instructions do not have dim
if (AMDGPU::getMIMGBaseOpcode(Opc)->BVH)
return true;

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 +5118,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
6 changes: 5 additions & 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,11 @@ def exp_vm : NamedBitOperand<"vm", "ExpVM">;
def FORMAT : CustomOperand<i8>;

def DMask : NamedIntOperand<i16, "dmask">;
def Dim : CustomOperand<i8>;

// The second argument (Optional) makes Dim optional for the AsmMatcher.
// This allows proper handling of the case where dim is missing. A validation
// function would ensure dim is present when required.
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