- 
                Notifications
    You must be signed in to change notification settings 
- Fork 99
CTS for subgroup_id language feature #4487
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
base: main
Are you sure you want to change the base?
Conversation
* Tests for subgroup_id and num_subgroups builtins * builtin validation * uniformity validation * execution tests
d563832    to
    9e5e7e0      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Did a dry run on Dawn CQ and everything is passing except the uniformity changes, which I've put a Tint CL up to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice overall.
Some requested changes, particularly the validation test should check that subgroup_id and num_subgroups are not supported (cause compile failure) when the  language feature is not supported.
| !t.params.use_struct, | ||
| 'missing @builtin(position) in the vertex output when the vertex output is not a struct' | ||
| ); | ||
| if (t.params.name === 'subgroup_id' || t.params.name === 'num_subgroups') { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of skipping the test for these cases, fail the test when the builtin is one of these and the feature is not supported.
One way to do this is to extend the table to include an optional field about which language feature (if any) supports the builtin.
| if (t.params.enable_extension) { | ||
| if (t.params.name.includes('subgroups')) { | ||
| code += 'enable subgroup;\n'; | ||
| if (t.params.name.includes('subgroup')) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By extending the info in the table, this code can be made more generic.
| module: t.device.createShaderModule({ | ||
| code: wgsl, | ||
| }), | ||
| entryPoint: 'main', | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the entryPoint field is optional when there's only one entry point.
CTS for gpuweb/gpuweb#5416
Issue: #
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.