Add declaration tests for WGSL immediate variables#4661
Conversation
7a7b534 to
a466dea
Compare
|
Results for build job (at 5e77072): -webgpu:shader,validation,decl,context_dependent_resolution:language_names:* - 4 cases, 12 subcases (~3/case)
+webgpu:shader,validation,decl,context_dependent_resolution:language_names:* - 5 cases, 15 subcases (~3/case)
+webgpu:shader,validation,decl,immediate:store_type,valid:* - 10 cases, 10 subcases (~1/case)
+webgpu:shader,validation,decl,immediate:store_type,invalid:* - 11 cases, 11 subcases (~1/case)
+webgpu:shader,validation,decl,immediate:scope:* - 2 cases, 2 subcases (~1/case)
+webgpu:shader,validation,decl,immediate:binding_attributes:* - 4 cases, 4 subcases (~1/case)
+webgpu:shader,validation,decl,immediate:access_mode:* - 4 cases, 4 subcases (~1/case)
+webgpu:shader,validation,decl,immediate:entry_point_interface:* - 5 cases, 5 subcases (~1/case)
+webgpu:shader,validation,decl,immediate:pointers:* - 8 cases, 8 subcases (~1/case)
-webgpu:shader,validation,decl,var:module_scope_types:* - 406 cases, 406 subcases (~1/case)
+webgpu:shader,validation,decl,var:module_scope_types:* - 464 cases, 464 subcases (~1/case)
-webgpu:shader,validation,decl,var:binding_point_on_non_resources:* - 8 cases, 8 subcases (~1/case)
+webgpu:shader,validation,decl,var:binding_point_on_non_resources:* - 12 cases, 12 subcases (~1/case)
-webgpu:shader,validation,decl,var:address_space_access_mode:* - 40 cases, 40 subcases (~1/case)
+webgpu:shader,validation,decl,var:address_space_access_mode:* - 48 cases, 48 subcases (~1/case)
-TOTAL: 283229 cases, 2325040 subcases
+TOTAL: 283344 cases, 2325157 subcases |
This patch adds a focused declaration suite for `var<immediate>`. The new tests cover accepted scalar, vector, matrix, and struct store types, rejected store types, module-scope-only declarations, rejection of binding attributes, rejection of explicit access modes, and the one-immediate-variable-per-entry-point rule. It also threads `immediate` through the existing declaration tests so the generic address space coverage checks the same declaration access rules as storage, uniform, private, workgroup, and function variables. The context-dependent resolution table now recognizes `immediate_address_space` as a language feature name.
a466dea to
a3c267f
Compare
|
@jrprice PTAL, catch a potential "issue" in chrome canary, it seems canary supported fixed array in immediate address space but the spec said it shouldn't. Fixed array could be supported in implementation but it doesn't be discussed in community yet. |
|
@jrprice softly ping for reviewing, thanks! |
| u32: { enable: ``, prelude: ``, type: `u32` }, | ||
| i32: { enable: ``, prelude: ``, type: `i32` }, | ||
| f32: { enable: ``, prelude: ``, type: `f32` }, | ||
| f16: { enable: `enable f16;`, prelude: ``, type: `f16` }, |
There was a problem hiding this comment.
Why is f16 here if it isn't supported yet?
There was a problem hiding this comment.
The spec text we landed seems to allow f16, but I can't find any discussion that says that this was intentional. That said it's probably easy enough to support since in Tint at least we just decompose the buffers anyway. I've asked internally whether this is expected to be supported or not.
@shaoboyan091 do we already have CTS execution/operation tests that cover f16 in immediate, to make sure they actually work?
There was a problem hiding this comment.
TBH, This is from this https://github.com/gpuweb/cts/blob/main/src/webgpu/api/operation/command_buffer/programmable/immediate.spec.ts#L410
But I suspect this is due to API limit #4297 and shouldn't affect decl test here. Let me change the code and try.
There was a problem hiding this comment.
After adding f16 support, local try report " error: using 'f16' in 'immediate' address space is not implemented yet" And I find in dawn part. https://source.chromium.org/chromium/chromium/src/+/main:third_party/dawn/src/tint/lang/wgsl/resolver/address_space_validation_test.cc;drc=874b76badc54f1ece2369347db39ee5406678021;l=756. That's a bit surprise me.
There was a problem hiding this comment.
I noticed the spec PR gpuweb/gpuweb#6297 about restrict f16 support, is this because we don't have implementation to verify the ability?
| t.expectCompileResult(false, wgsl); | ||
| }); | ||
|
|
||
| g.test('scope') |
There was a problem hiding this comment.
Given the test is mostly predicated on the module vs function can we just make 2 tests?
There was a problem hiding this comment.
Yeah the code paths are too different for module-scope to be acting as a control-case here.
Suggest dropping module-scope since it's covered by other tests, and just having function scope where the control case uses the function address space.
The WGSL spec allows f16 in the immediate address space: f16 is a host-shareable constructible scalar, and immediate store types only exclude arrays and structures containing arrays. Remove the f16 immediate skip so the validation tests assert the spec-correct result (f16 immediate compiles). An implementation that rejects f16 immediate is non-conformant and will be flagged by these tests.
The WGSL spec is restricting f16 in the immediate address space (gpuweb/gpuweb#6297), so move f16 and vec3h to the rejected immediate store types and exclude f16 types from the accepted immediate store types in the shared module-scope coverage. Simplify the scope test to declare the variable at function scope for both cases and vary only the address space, using the function address space as the control. Module-scope validity is already covered by the module_scope_types test.
|
Discussing internally, we think |
Good to know this. F16 is important. |
The WGSL spec allows f16 in the immediate address space, so f16 and vec3h are valid immediate store types and are expected to compile. No skip is added for implementations that reject f16 immediate data; an implementation that does so is non-conformant, and these tests flag it.
Adds a focused declaration suite for
var<immediate>.The new tests cover accepted scalar, vector, matrix, and struct store types, rejected store types, module-scope-only declarations, rejection of binding attributes, rejection of explicit access modes, and the one-immediate-variable-per-entry-point rule.
It also threads
immediatethrough the existing declaration tests so the generic address space coverage checks the same declaration access rules as storage, uniform, private, workgroup, and function variables. The context-dependent resolution table now recognizesimmediate_address_spaceas a language feature name.Notes for reviewers:
immediate_address_spaceWGSL language feature, so they run only where therequires immediate_address_space;shaders can compile (not merely where the immediate API surface is exposed).f16/vec3hare treated as valid immediate store types (host-shareable and constructible, gated onshader-f16), consistent with howuniformaccepts them.Issue: #
Requirements for PR author:
.unimplemented()./** documented */and new helper files are found inhelper_index.txt.Requirements for reviewer sign-off: