-
Notifications
You must be signed in to change notification settings - Fork 260
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
Add SPV_NV_cooperative_matrix2 and SPV_NV_tensor_addressing #458
Conversation
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 think only the quantifier suggestions are truly out of sync with the text, the rest is more nitpicking
{ "kind" : "MemoryAccess", "name" : "'Memory Operand'"}, | ||
{ "kind" : "TensorAddressingOperands", "name" : "'Tensor Addressing Operands'"} |
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.
According to extension text
{ "kind" : "MemoryAccess", "name" : "'Memory Operand'"}, | |
{ "kind" : "TensorAddressingOperands", "name" : "'Tensor Addressing Operands'"} | |
{ "kind" : "MemoryAccess", "name" : "'Memory Operand'", "quantifier" : "?"}, | |
{ "kind" : "TensorAddressingOperands", "name" : "'Tensor Addressing Operands'", "quantifier" : "?"} |
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'm not clear on the effect of this quantifier change. The MemoryAccess and TensorAddressingOperands masks are required, but additional literal values are optional and depend on those flags. So I think this should not be considered optional? I'm also not sure it actually has any effect in jsonToSpirv.cpp based on the "kind".
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.
Oh right, other ops make the absence of MemoryAccess
equivalent to setting None
(and that's modelled this way) and in the extension OpCooperativeMatrixLoadTensorNV
and OpCooperativeMatrixStoreTensorNV
have 6+ and 4+ words respectively, so I made assumption they could also be omitted (I was actually going to make a comment about the wording on these operands). So the extension should probably says 8+ and 6+ then.
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.
Agreed, I will make that change.
{ "kind" : "MemoryAccess", "name" : "'Memory Operand'"}, | ||
{ "kind" : "TensorAddressingOperands", "name" : "'Tensor Addressing Operands'"} |
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.
{ "kind" : "MemoryAccess", "name" : "'Memory Operand'"}, | |
{ "kind" : "TensorAddressingOperands", "name" : "'Tensor Addressing Operands'"} | |
{ "kind" : "MemoryAccess", "name" : "'Memory Operand'", "quantifier" : "?"}, | |
{ "kind" : "TensorAddressingOperands", "name" : "'Tensor Addressing Operands'", "quantifier" : "?"} |
Co-authored-by: Victor Lomuller <[email protected]>
Merging as discussed in the October 23rd teleconference. |
No description provided.