Add shader validation tests on subgroup-size-control - Part I#4641
Add shader validation tests on subgroup-size-control - Part I#4641Jiawei-Shao wants to merge 2 commits into
subgroup-size-control - Part I#4641Conversation
This patch adds the first part of the shader validation tests on the extension `subgroup-size-control`: - Enabling `subgroup_size_control` must also enable `subgroups`. - The WGSL attribute `@subgroup_size` must be used when `subgroup_size_control` is enabled. - `@subgroup_size` can only be used in the compute stage issue: gpuweb#4640
|
Results for build job (at 1756791): +webgpu:shader,validation,extension,subgroup_size_control:enable_subgroup_size_control_requires_subgroups:* - 2 cases, 2 subcases (~1/case)
+webgpu:shader,validation,extension,subgroup_size_control:use_subgroup_size_attribute_requires_subgroup_size_control_extension_enabled:* - 2 cases, 2 subcases (~1/case)
+webgpu:shader,validation,extension,subgroup_size_control:subgroup_size_attribute_only_valid_in_compute_stage:* - 3 cases, 3 subcases (~1/case)
-TOTAL: 281247 cases, 2322858 subcases
+TOTAL: 281254 cases, 2322865 subcases |
|
PTAL, thanks! |
| * Returns a subgroup size value that is valid for use in the @subgroup_size | ||
| * attribute on the current adapter. | ||
| * | ||
| * On Intel gen-12lp, subgroupMinSize may be 8 in fragment stages, which is below the allowed range |
There was a problem hiding this comment.
You're saying adapterInfo.subgroupMinSize is 8 on these devices, but 8 is not actually a valid subgroup size in compute? That's kind of weird, it requires applications to do weird stuff like this to get the right subgroup size. I would have thought subgroupMinSize/subgroupMaxSize are just for compute, especially now that we're adding subgroup-size-control which only applies to compute.
I understand we're saying not all values between subgroupMinSize and subgroupMaxSize are valid to create all pipelines with, but surely they should all work for a trivial compute pipeline? Maybe we can't technically specify that, but I think we should try to test it unless there are known counterexamples.
There was a problem hiding this comment.
I believe it is true that on these devices, the subgroup size is 8 for fragment shaders but that is never a valid subgroup size for a compute shader. This is the reason we decided to say that it would be a spurious failure if you requested a subgroup size that is not supported (otherwise we have to add a second set of limits as previously proposed, which we decided against).
So no, unfortunately not all sizes between min and max will work even for trivial compute pipelines.
| t.expectCompileResult( | ||
| enableExtension, | ||
| ` | ||
| ${enableExtension ? 'enable subgroups; enable subgroup_size_control;' : ''} |
There was a problem hiding this comment.
Tighten this so we're changing as little as possible between the true and false cases.
| ${enableExtension ? 'enable subgroups; enable subgroup_size_control;' : ''} | |
| enable subgroups; | |
| ${enableExtension ? 'enable subgroup_size_control;' : ''} |
| .fn(t => { | ||
| const { enableExtension } = t.params; | ||
|
|
||
| const subgroupSize = getValidSubgroupSizeForSubgroupSizeAttribute(t.device.adapterInfo); |
There was a problem hiding this comment.
Does compiling a shader really require the subgroup size to be valid for the device? It seems like it should work for any size, and only be validated at pipeline creation.
There was a problem hiding this comment.
Agreed, I don't see anything in the spec PR that suggests that the devices constraints would cause a shader-creation error, just the power-of-two and multiple-of-workgroup-x-dim things which are not dependent on device. So in theory you could just pick something like 32 here for the purposes of these tests.
| ); | ||
| }); | ||
|
|
||
| const kStageShaders = { |
There was a problem hiding this comment.
nit: If these are only going to be used by one test, put them inside the test.
This patch adds the first part of the shader validation tests on the extension
subgroup-size-control:subgroup_size_controlmust also enablesubgroups.@subgroup_sizemust be used whensubgroup_size_controlis enabled.@subgroup_sizecan only be used in the compute stageIssue: #4640
Requirements for PR author:
.unimplemented()./** documented */and new helper files are found inhelper_index.txt.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.