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.

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

enums should always be parseable; check against versions, capabilities, and extensions during validation instead #5364

Closed
dneto0 opened this issue Aug 8, 2023 · 2 comments · Fixed by #5370
Assignees

Comments

@dneto0
Copy link
Collaborator

dneto0 commented Aug 8, 2023

Currently, SPIRV-Tools parsing only makes an enum visible if it's in some version of SPIR-V, or is guarded by a capability or extension.
See
https://github.com/KhronosGroup/SPIRV-Tools/blob/main/source/operand.cpp#L66

  // We consider the current operand as available as long as
  // 1. The target environment satisfies the minimal requirement of the
  //    operand; or
  // 2. There is at least one extension enabling this operand; or
  // 3. There is at least one capability enabling this operand.
  //
  // Note that the second rule assumes the extension enabling this operand
  // is indeed requested in the SPIR-V code; checking that should be
  // validator's work.

See also this proposal to enforce this in SPIRV-Headers.
KhronosGroup/SPIRV-Headers#369

Arguably this is the wrong thing to do. In the case of SPV_KHR_cooperative_matrix, it's been argued that the enums like MatrixASignedComponentsKHR in CooperativeMatrixOperands should not have to be specially guarded because they are only used by an instruction which itself is already guarded by a capability.

The parser doesn't have that context, so it can't check the transitive dependency, but the transitive dependency should be sufficient.
The proposal is that the validator should be the one to check, instead of the parser.

Going with this proposal would dovetail nicely with #5332 which further removes version checks in grammar operations.

@dneto0
Copy link
Collaborator Author

dneto0 commented Aug 8, 2023

cc: @alan-baker

@dneto0 dneto0 self-assigned this Aug 8, 2023
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Aug 9, 2023
A token is allowed to parse even when it's from the wrong
version, or is not enabled by a capability or extension.
This allows more modules to parse.

Version/capability/extension checking is fully moved to
validation instead.

Fixes: KhronosGroup#5364
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Aug 9, 2023
A token is allowed to parse even when it's from the wrong
version, or is not enabled by a capability or extension.
This allows more modules to parse.

Version/capability/extension checking is fully moved to
validation instead.

Fixes: KhronosGroup#5364
@dneto0
Copy link
Collaborator Author

dneto0 commented Aug 9, 2023

This is going well.

dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Aug 9, 2023
A token is allowed to parse even when it's from the wrong
version, or is not enabled by a capability or extension.
This allows more modules to parse.

Version/capability/extension checking is fully moved to
validation instead.

Fixes: KhronosGroup#5364
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Aug 10, 2023
A token is allowed to parse even when it's from the wrong
version, or is not enabled by a capability or extension.
This allows more modules to parse.

Version/capability/extension checking is fully moved to
validation instead.

Fixes: KhronosGroup#5364
dneto0 added a commit that referenced this issue Aug 10, 2023
A token is allowed to parse even when it's from the wrong
version, or is not enabled by a capability or extension.
This allows more modules to parse.

Version/capability/extension checking is fully moved to
validation instead.

Fixes: #5364
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant