[naga spv-out] Handle matCx2 in uniform buffers #8240

jamienicol · 2025-09-18T10:11:23Z

Connections
SPIR-V fix for #4371 (leave open for GLSL)

Description
Vulkan's default uniform buffer "extended layout" (std140) defines matrices in terms of arrays of their vector type, and defines arrays to have an alignment rounded up to a multiple of 16. WGSL/Naga IR defines matrices to have the same alignment as their vector type, not rounded up. For a matCx2 this means Naga expects each column to have an alignment of 8, whereas Vulkan expects 16.

To fix this, for each matCx2 struct member in a uniform buffer we decompose matrix such that each column is a vector member of the containing struct. For matrices used directly as uniform buffers, we create a struct that contains a vector member for each column. For arrays of matrices, we declare an array of a struct containing a vector member for each column.

We only use these alternative type declarations for uniform buffers, and when loading a value of such a type we convert it to the original type. This is in contrast to the approach used in the HLSL backend where all structs containing matCx2 have a modified layout. This allows the HLSL backend to avoid having to convert a struct after loading one, but it still must convert matrices or arrays of matrices when loading from uniform buffers, or when accessing values of these types that are struct members in any address space. The approach taken here means we consistently must convert two-row matrices, or arrays or structs containing two-row matrices, but only when loading from uniform buffers. It also allows us to ignore stores altogether, as uniform buffers are read only.

Testing
Extended struct_layout tests to handle additional cases, and remove expected fail for vulkan. Made existing hlsl_mat_cx[23].wgsl snapshot tests run on SPIR-V too.

Squash or Rebase?

Squash (making sure commit message is tidy)

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.

jamienicol · 2025-09-18T10:58:25Z

@jimblandy I know we previously discussed my approach here of using alternative type declarations for uniform types, vs always declaring structs with a modified type, and you had a preference for the latter. After testing out both I found this approach much cleaner. There was less special casing as the rules for when we need to convert a type are more consistent, and being able to ignore stores altogether makes life much easier. (And FWIW I'm not convinced stores are implemented correctly in HLSL, but I still need to test that and file a bug if I'm right). I've tried to document that decision both in this PR and in the module-level comment.)

Extend existing tests to use dynamic indexing of matrices' columns as well static indexing. Add new tests for arrays of matrices.

Two-row matrices in uniform buffers in Vulkan/SPIR-V's default "extended" (std140) layout do not adhere to WGSL/Naga IR's layout rules - each column is aligned to a minimum of 16 bytes rather than just the vector size. To work around this, we emit additional std140 compatible type declarations for each type used in a uniform buffer that requires one. Any two-row matrix struct member is decomposed into the containing struct as separate vectors for each column. Two-row matrices used directly as uniform buffers are wrapped in a struct with a vector member for each column, and arrays (of arrays, etc) of two-row matrices are declared as arrays (of arrays) of structs containing a vector member for each column. When loading such a value from a uniform buffer, we convert from the std140 compatible type to the regular type immediately after loading. Accesses of a uniform two-row matrix's columns using constant indices are rewritten to access the containing struct's vector members directly. Accesses using dynamic indices are implemented by loading and converting the matrix, extracting each column then `switch`ing on the index to select the correct column. We can now remove the expected failure annotation for the Vulkan backend on the uniform_input struct_layout test, as it now passes. We additionally make the hlsl_mat_cx*.wgsl snapshot tests run for the SPIR-V backend too, and remove the "hlsl" prefix from the name, as they are no longer just relevant for HLSL.

andyleiserson

Looks... well, I hesitate to say "good" not because of anything about the PR, but because the Cx2 handling is such a pain 😀. But seriously, thank you for taking care of this. This approach does seem less painful than the way that HLSL handles it.

I went looking for which tests are covering the case of accesses to struct members after a Cx2 matrix that is flattened into the struct. It seems like this is at least covered by the f16 test, but I don't see a test explicitly targeting this. I don't think it's necessary to add one, but it might be worth putting a note in the f16 test that it's providing this coverage.

andyleiserson · 2025-10-07T21:05:30Z

tests/tests/wgpu-gpu/shader/struct_layout.rs

+
+            // https://github.com/gfx-rs/wgpu/issues/4371
+            let failures = if storage_type == InputStorageType::Uniform && rows == 2 {
+                Backends::GL


It might be worth adding a comment where UNIFORM_INPUT is defined mentioning that some specific cases are disabled on the GL backend, since that's normally where an expected-failure would be declared.

Done in 5bdee64

andyleiserson · 2025-10-07T21:14:46Z

naga/tests/out/spv/wgsl-access.spvasm

+OpMemberName %49 2 "col2"
+OpMemberName %49 3 "col3"
+OpName %49 "std140_mat4x2<f32>"
+OpName %50 "std140_array<mat4x2<f32>, 2>"


thought: Despite being technically valid, does having spaces in the debug names increase the risk of interoperability problems?

Hrm, I'm not sure to be honest. I don't see why a driver or program would care, but you never know. Would you be more comfortable if I sanitized the names, or just remove them altogether? Or should we land as is and wait and see if we see any issues?

I guess ideally we'd live in this promised land and could easily format type names for SPIRV. Presumably based on the SPIRV-tools FriendlyNameMapper.

I think it's fine to leave as-is and wait until an issue comes up. The debug stuff should not be turned on in default configs so the risk is limited.

andyleiserson · 2025-10-07T21:35:52Z

naga/tests/in/wgsl/mat_cx2.toml

Adding spv.debug = true to this might make it easier to inspect the results (thus more likely any regressions get caught during review).

Done in 900bde9

andyleiserson · 2025-10-07T21:56:57Z

naga/src/back/spv/writer.rs

+            block,
+            Instruction::switch(
+                column_index_param_id,
+                cases.last().unwrap().label_id,


Is this assuming BoundsCheckPolicy::Restrict?

yes it was. I've pushed 9925f12 which handles the different policies

…iling on GL

jamienicol · 2025-10-08T10:58:03Z

Looks... well, I hesitate to say "good" not because of anything about the PR, but because the Cx2 handling is such a pain 😀.

hehe.

I went looking for which tests are covering the case of accesses to struct members after a Cx2 matrix that is flattened into the struct. It seems like this is at least covered by the f16 test, but I don't see a test explicitly targeting this. I don't think it's necessary to add one, but it might be worth putting a note in the f16 test that it's providing this coverage.

I think an explicit tests makes sense now you mention it. It's easy enough to write. Done in 85656cc

jamienicol · 2025-10-08T16:59:16Z

@andyleiserson I assume you're happy with the changes since you pre-approved? I think the only possibly contentious one was the bounds checking. @jimblandy anything to add? If not I'll squash and merge this tomorrow morning

andyleiserson · 2025-10-09T17:31:57Z

@jimblandy and I talked about this in our 1:1 today and some questions came up, which I think all revolved around a difference between SPIR-V and HLSL that I hadn't fully understood until thinking about it more after Jim and I talked.

HLSL always lays out these matrices with 16-byte stride. This means that a fixup is needed for both storage and uniform buffers. However, the strategy for storage and uniform buffers is quite different, because storage buffers are accessed as ByteAddressBuffers.

SPIR-V has the capability to describe 8-byte stride for these matrices, except in the case that they appear in uniform buffers, where the Vulkan std140 extended layout applies and a 16-byte stride is required. Which is why the approach of defining an alternate struct used solely for the case of accessing uniform buffers makes sense.

I was briefly concerned but am back to being happy with the changes. However, this has been a reminder that there's a lot of complexity of Vulkan and SPIR-V that I don't understand, so @jimblandy if you have time to glance over this change that would be valuable.

jamienicol · 2025-10-09T17:36:19Z

@andyleiserson if you have any additions to the module level comment that you think would help make this clearer for others (or ourselves) in the future please let me know (or just push an extra commit to the PR if that's easier)

andyleiserson

Some suggestions for the docs.

One thing that seems like a potential source of confusion is the naming of the uniform buffer types std140. This could be interpreted as "this type laid out according to std140 rules", but it's really "a modified version of the type necessary because std140 would not lay out the type the way we want". I haven't come up with an alternative that I really like, and I don't think this is that important since the debug names aren't used for much, but I thought I would mention it.

andyleiserson · 2025-10-09T17:55:08Z

naga/src/back/spv/mod.rs

+WGSL's ["Internal Layout of Values"][ilov] rules specify how each WGSL type
+should be stored in `uniform` and `storage` buffers, and Naga IR adheres to
+these rules. The SPIR-V we generate must access values in that form, even when
+it is not what Vulkan would use normally. Fortunately the rules for `storage`
+buffers match Vulkan's, but some adjustments must be made when emitting SPIR-V
+for `uniform` buffers.


Suggested change

WGSL's ["Internal Layout of Values"][ilov] rules specify how each WGSL type

should be stored in `uniform` and `storage` buffers, and Naga IR adheres to

these rules. The SPIR-V we generate must access values in that form, even when

it is not what Vulkan would use normally. Fortunately the rules for `storage`

buffers match Vulkan's, but some adjustments must be made when emitting SPIR-V

for `uniform` buffers.

WGSL's ["Internal Layout of Values"][ilov] rules specify the memory layout of

each WGSL type. The memory layout is important for data stored in `uniform` and

`storage` buffers, especially when exchanging data with CPU code.

Both WGSL and Vulkan specify some conditions that a type's memory layout

must satisfy in order to use that type in a `uniform` or `storage` buffer.

For `storage` buffers, the WGSL and Vulkan restrictions are compatible, but

for `uniform` buffers, WGSL allows some types that Vulkan does not, requiring

adjustments when emitting SPIR-V for `uniform` buffers.

andyleiserson · 2025-10-09T18:04:51Z

naga/src/back/spv/mod.rs

+In Vulkan's ["extended layout"][extended-layout] (also known as std140) used
+for `uniform` buffers, matrices are defined in terms of arrays of their vector
+type, and arrays are defined to have an alignment equal to the alignment of
+their element type rounded up to a multiple of 16. This means that each column
+of the vector has a minimum alignment of 16. WGSL, and consequently Naga IR, on
+the other hand defines each column to have an alignment equal to the alignment
+of the vector type, without being rounded up to 16.


Suggested change

In Vulkan's ["extended layout"][extended-layout] (also known as std140) used

for `uniform` buffers, matrices are defined in terms of arrays of their vector

type, and arrays are defined to have an alignment equal to the alignment of

their element type rounded up to a multiple of 16. This means that each column

of the vector has a minimum alignment of 16. WGSL, and consequently Naga IR, on

the other hand defines each column to have an alignment equal to the alignment

of the vector type, without being rounded up to 16.

SPIR-V provides detailed control over the layout of matrix types, and is

capable of describing the WGSL memory layout. However, Vulkan imposes

additional restrictions.

Vulkan's ["extended layout"][extended-layout] (also known as std140) rules

apply to types used in `uniform` buffers. Under these rules, matrices are

defined in terms of arrays of their vector type, and arrays are defined to have

an alignment equal to the alignment of their element type rounded up to a

multiple of 16. This means that each column of the matrix has a minimum

alignment of 16. WGSL, and consequently Naga IR, on the other hand specifies

column alignment equal to the alignment of the vector type, without being

rounded up to 16.

jamienicol marked this pull request as draft September 18, 2025 10:16

jamienicol force-pushed the matCx2-uniform-spv branch 2 times, most recently from f407879 to bbac5f3 Compare September 18, 2025 10:42

jamienicol marked this pull request as ready for review September 18, 2025 10:58

andyleiserson self-assigned this Sep 24, 2025

jamienicol added 2 commits October 6, 2025 10:03

[test] Add additional matrix struct_layout tests

8ef1922

Extend existing tests to use dynamic indexing of matrices' columns as well static indexing. Add new tests for arrays of matrices.

jamienicol force-pushed the matCx2-uniform-spv branch from bbac5f3 to 06e89f3 Compare October 6, 2025 09:17

andyleiserson approved these changes Oct 7, 2025

View reviewed changes

jamienicol added 6 commits October 8, 2025 11:52

add comment noting that some unfiform input subtests are marked as fa…

5bdee64

…iling on GL

add struct layout test for matCx2 followed by another struct member

85656cc

fix doc typo

10e18c5

update stale references to hlsl_mat_cxN.wgsl

e1ae0db

set spv.debug = true for matcx2 snapshot test

900bde9

handle different bounds check policies

9925f12

andyleiserson approved these changes Oct 9, 2025

View reviewed changes

andyleiserson mentioned this pull request Oct 9, 2025

Matrix of the form matCx2 not getting translated properly when in a uniform buffer #4371

Open

-WGSL's ["Internal Layout of Values"][ilov] rules specify how each WGSL type
-should be stored in `uniform` and `storage` buffers, and Naga IR adheres to
-these rules. The SPIR-V we generate must access values in that form, even when
-it is not what Vulkan would use normally. Fortunately the rules for `storage`
-buffers match Vulkan's, but some adjustments must be made when emitting SPIR-V
-for `uniform` buffers.
+WGSL's ["Internal Layout of Values"][ilov] rules specify the memory layout of
+each WGSL type. The memory layout is important for data stored in `uniform` and
+`storage` buffers, especially when exchanging data with CPU code.
+Both WGSL and Vulkan specify some conditions that a type's memory layout
+must satisfy in order to use that type in a `uniform` or `storage` buffer.
+For `storage` buffers, the WGSL and Vulkan restrictions are compatible, but
+for `uniform` buffers, WGSL allows some types that Vulkan does not, requiring
+adjustments when emitting SPIR-V for `uniform` buffers.

-In Vulkan's ["extended layout"][extended-layout] (also known as std140) used
-for `uniform` buffers, matrices are defined in terms of arrays of their vector
-type, and arrays are defined to have an alignment equal to the alignment of
-their element type rounded up to a multiple of 16. This means that each column
-of the vector has a minimum alignment of 16. WGSL, and consequently Naga IR, on
-the other hand defines each column to have an alignment equal to the alignment
-of the vector type, without being rounded up to 16.
+SPIR-V provides detailed control over the layout of matrix types, and is
+capable of describing the WGSL memory layout. However, Vulkan imposes
+additional restrictions.
+Vulkan's ["extended layout"][extended-layout] (also known as std140) rules
+apply to types used in `uniform` buffers. Under these rules, matrices are
+defined in terms of arrays of their vector type, and arrays are defined to have
+an alignment equal to the alignment of their element type rounded up to a
+multiple of 16. This means that each column of the matrix has a minimum
+alignment of 16. WGSL, and consequently Naga IR, on the other hand specifies
+column alignment equal to the alignment of the vector type, without being
+rounded up to 16.

[naga spv-out] Handle matCx2 in uniform buffers #8240

Are you sure you want to change the base?

[naga spv-out] Handle matCx2 in uniform buffers #8240

Uh oh!

Conversation

jamienicol commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamienicol commented Sep 18, 2025

Uh oh!

andyleiserson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienicol commented Oct 8, 2025

Uh oh!

jamienicol commented Oct 8, 2025

Uh oh!

andyleiserson commented Oct 9, 2025

Uh oh!

jamienicol commented Oct 9, 2025

Uh oh!

andyleiserson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jamienicol commented Sep 18, 2025 •

edited

Loading