Skip to content

[mlir][spirv] Fix coop matrix load #65712

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

Merged
merged 2 commits into from
Sep 8, 2023
Merged

[mlir][spirv] Fix coop matrix load #65712

merged 2 commits into from
Sep 8, 2023

Conversation

kuhar
Copy link
Member

@kuhar kuhar commented Sep 8, 2023

  • Fix order of operands/attributes
  • Allow for stride to be any integer type
  • Use ODS for parsing/printing
  • Update examples and tests
  • Fix a typo in SPIR-V tblgen code

- Allow for stride to be any integer type
- Use ODS for parsing/printing
- Update examples and tests
@kuhar kuhar requested a review from a team as a code owner September 8, 2023 04:43
@github-actions github-actions bot added mlir:core MLIR Core Infrastructure mlir labels Sep 8, 2023
Copy link
Contributor

@qedawkins qedawkins left a comment

Choose a reason for hiding this comment

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

Looking at the spec the stride seems to be optional: https://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/master/extensions/KHR/SPV_KHR_cooperative_matrix.html
unless I am misinterpreting it. Should we try to match the optionality in the spec here?

@kuhar
Copy link
Member Author

kuhar commented Sep 8, 2023

Looking at the spec the stride seems to be optional: https://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/master/extensions/KHR/SPV_KHR_cooperative_matrix.html unless I am misinterpreting it. Should we try to match the optionality in the spec here?

Matching this two-level optionality (operand and attribute) is not possible without changes on the tablegen side (

if (i != e - 1) {
PrintFatalError(loc, "SPIR-V ops can have Variadic<..> or "
"std::optional<...> arguments only if "
"it's the last argument");
}
). Would be nice to have so that we can deserialize SPIR-V blobs that rely on this, but I think having it as non-optional simplifies things.

@kuhar kuhar requested a review from a team as a code owner September 8, 2023 15:41
@kuhar kuhar requested a review from qedawkins September 8, 2023 15:43
Copy link
Contributor

@qedawkins qedawkins left a comment

Choose a reason for hiding this comment

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

I see, leaving it as TODO makes sense then. Thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants