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

Fix hlsl storage format generation #6993

Merged
merged 13 commits into from
Jan 31, 2025
Merged

Conversation

Vecvec
Copy link
Contributor

@Vecvec Vecvec commented Jan 26, 2025

Connections

Description
Previously we generated a vec2 for rg8unorm on hlsl as the storage type; however, this is for the output variable, not the stored type. This PR makes this always a vec4 unless it's a one length type because: 1. They seem to be auto convertible to a vec4 2. it breaks image atomics. I would prefer these to be vec4s too but I can't find a way to not break image atomics.

Testing
Adds a new naga snapshot which loads and stores to storage textures which are r8unorm, rg8unorm and rgba8unorm.

Checklist

  • Run cargo fmt.
  • [n/a] Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@Vecvec Vecvec requested a review from a team January 26, 2025 06:30
@cwfitzgerald
Copy link
Member

Have you tested this on an example that is actually run?

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

The behavior outlined in the WGSL spec matches the behavior documented for HLSL.

https://www.w3.org/TR/WGSL/#storage-texel-formats

https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-to-type#default-values-for-missing-components-in-a-texture

So, this looks correct overall but I'm not sure about the one component formats. Does the implicit cast insert a 1 for the last component (alpha)?

@Vecvec Vecvec requested a review from a team as a code owner January 30, 2025 15:13
@Vecvec
Copy link
Contributor Author

Vecvec commented Jan 30, 2025

Does the implicit cast insert a 1 for the last component (alpha)?

I haven't tested what happens, I could figure this out but it could take some time. I wanted to prevent compile errors before worrying about that.

@Vecvec
Copy link
Contributor Author

Vecvec commented Jan 31, 2025

Have you tested this on an example that is actually run?

Added a test for this.

@Vecvec
Copy link
Contributor Author

Vecvec commented Jan 31, 2025

It shouldn't be to hard to have some special logic in textureLoad/Store for one length vectors if that is wanted. I think the driver's version should never be much faster than in HLSL. If wanted I could add that.

@teoxoy
Copy link
Member

teoxoy commented Jan 31, 2025

That could be great, or we could get those working with 4 component vectors and finding a way to still make image atomics work?

I think this PR is good to land as is though.

@teoxoy teoxoy merged commit 7cde470 into gfx-rs:trunk Jan 31, 2025
32 checks passed
@Vecvec
Copy link
Contributor Author

Vecvec commented Jan 31, 2025

we could get those working with 4 component vectors and finding a way to still make image atomics work?

I think we could if we don't allow mixing of normal load/stores with atomic operations as these require different formats.

@cwfitzgerald cwfitzgerald deleted the hlsl-storage-format branch January 31, 2025 23:27
@teoxoy
Copy link
Member

teoxoy commented Feb 3, 2025

Right, that doesn't sound like it's worth the cost. We probably have to handle the scalar <-> vec translation when we emit the image operations (as you mentioned).

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