Skip to content

Add shader validation tests on subgroup-size-control - Part I#4641

Open
Jiawei-Shao wants to merge 2 commits into
gpuweb:mainfrom
Jiawei-Shao:test-subgroup-size-control
Open

Add shader validation tests on subgroup-size-control - Part I#4641
Jiawei-Shao wants to merge 2 commits into
gpuweb:mainfrom
Jiawei-Shao:test-subgroup-size-control

Conversation

@Jiawei-Shao
Copy link
Copy Markdown
Collaborator

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: #4640


Requirements for PR author:

  • [*] All missing test coverage is tracked with "TODO" or .unimplemented().
  • [*] New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • [*] Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)
  • [*] Test have be tested with compatibility mode validation enabled and behave as expected. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located.
  • Test descriptions are accurate and complete.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Tests avoid over-parameterization (see case count report).

When landing this PR, be sure to make any necessary issue status updates.

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
@github-actions
Copy link
Copy Markdown

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

@Jiawei-Shao
Copy link
Copy Markdown
Collaborator Author

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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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;' : ''}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tighten this so we're changing as little as possible between the true and false cases.

Suggested change
${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);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 = {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: If these are only going to be used by one test, put them inside the test.

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