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

[SPIR-V 1.1] SPIRVReader: Add OpSizeOf support #2853

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

svenvh
Copy link
Member

@svenvh svenvh commented Nov 11, 2024

The OpSizeOf instruction was added in SPIR-V 1.1, but not supported yet.

The `OpSizeOf` instruction was added in SPIR-V 1.1, but not supported
yet.
Copy link
Contributor

@vmaksimo vmaksimo left a comment

Choose a reason for hiding this comment

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

LGTM
Do we some day expect to have forward translation as well?

Comment on lines +3 to +4
; TODO: reenable spirv-val when OpSizeOf is supported.
; R/U/N: spirv-val %t.spv
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there already an issue in SPIRV-Tools?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, spirv-val doesn't know much about OpSizeOf yet. I'm preparing a PR to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just an issue reported to SPIR-V Tolls would have been good enough for now, but that's great your fix is already in progress :)

Copy link
Member Author

Choose a reason for hiding this comment

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

For reference, the fix is here: KhronosGroup/SPIRV-Tools#5879
(but even if that lands soon, we'll need to wait for the next SPIRV-Tools release before we can re-enable this check in CI.)

@svenvh
Copy link
Member Author

svenvh commented Nov 11, 2024

Do we some day expect to have forward translation as well?

Some day hopefully. For now I'm focusing on the consumption side only.

@MrSidims MrSidims merged commit 9aeb7eb into KhronosGroup:main Nov 13, 2024
9 checks passed
@svenvh svenvh deleted the opsizeof branch November 13, 2024 10:20
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 this pull request may close these issues.

3 participants