Skip to content

[naga spv-out] Add f16 io polyfill #7884

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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

cryvosh
Copy link
Contributor

@cryvosh cryvosh commented Jul 5, 2025

Connections
Extension of #7656

Description
Polyfills f16 I/O when storageInputOutput16 capability is missing on vulkan backend.
Emits these variables as f32 and inserts OpFConverts at loads/stores.
If capability is available, generated spirv is unaffected.

Testing
Added a snapshot test and some basic inspection of the spirv disassembly in spirv_capabilities.rs.
Generated spirv in snapshots look okay to me, but I haven't yet run any live correctness tests.

Squash or Rebase?
Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@cryvosh cryvosh marked this pull request as ready for review July 5, 2025 18:22
@cryvosh cryvosh requested a review from a team as a code owner July 5, 2025 18:22
@cryvosh cryvosh changed the title Add f16 io polyfill [naga spv-out] Add f16 io polyfill Jul 5, 2025
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but I noticed some things that seem like potential issues.

I also think the API could use some work, but let's iterate on that in follow-up PRs.

Comment on lines +44 to +48
pub fn register_variable(&mut self, variable_id: Word, f32_type_id: Word, f16_type_id: Word) {
self.variable_map
.insert(variable_id, (f32_type_id, f16_type_id));
}
Copy link
Member

Choose a reason for hiding this comment

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

question: I noticed that f16_type_id is never used when recalled via get_polyfill_info. Is this a bug, or just some data that can be eliminated?

converted,
&mut prelude.body,
);
id = converted;
Copy link
Member

Choose a reason for hiding this comment

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

question: You have an id = converted; statement here, but not in the other branch where we handle a Struct case as fallback. Should that be there, too?

@ErichDonGubler ErichDonGubler added type: enhancement New feature or request backend: vulkan Issues with Vulkan lang: SPIR-V Vulkan's Shading Language labels Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: vulkan Issues with Vulkan lang: SPIR-V Vulkan's Shading Language type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants