-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[AMDGPU][MC] Improve error message for missing dim operand #96588
Changes from 2 commits
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 |
---|---|---|
|
@@ -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>; | ||
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. Can you comment the parameter name here? I have to look to see what this does 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. Done. 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 mean like /OperandName=/ not exposition before the definition 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. also can call it true instead of 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. Updated comment as suggested. |
||
|
||
def dst_sel : SDWAOperand<"dst_sel", "SDWADstSel">; | ||
def src0_sel : SDWAOperand<"src0_sel", "SDWASrc0Sel">; | ||
|
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.
can this be a named feature instead of a generation check
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.
I was not able to find a named feature for "dim". Note that
isGFX10Plus()
etc are defined as member functions of the classAMDGPUAsmParser
and are used throughout the file. For example, the first few lines ofparseDim()
include a call ofisGFX10Plus()
: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.
Right, you would need to add it. Or at least hide this in an alias that shows why the version matters