-
Notifications
You must be signed in to change notification settings - Fork 86
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
Adds validation tests for count* builtin functions. #3498
Conversation
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.
My advice: you don't have to test the good cases such as AbstractInt because that should already be covered by the execution cases.
regarding apparent duplication / DRY, we discussed and decided the duplication is useful because it makes it trivial to see and verify coverage for each given builtin.
src/webgpu/shader/validation/expression/call/builtin/countLeadingZeros.spec.ts
Outdated
Show resolved
Hide resolved
src/webgpu/shader/validation/expression/call/builtin/countLeadingZeros.spec.ts
Outdated
Show resolved
Hide resolved
Updated the tests according to comments in #3565. PTAL! (Also adding Dan as a reviewer since a lot of those comments were from him.) |
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.
Thanks! Sorry for the delay
Piggy-backing off of Brandon's PR at , also implementing something a bit simpler to test the waters. As Brandon points out in his PR for min/max, some tests (these included) are VERY similar, to a point where pretty much the only difference is the first line of the test files, so I'm also wondering whether this is fine or whether there's precedence for what to do.
More specific to this suite of tests though, the WGSL spec for these count* builtins does not explicitly state that AbstractInts are a part of the parameterization, but during my testing, I noticed that passing an AbstractInt does not fail compilation because of how overload resolution handles it. As a result, I decided to not have a specific test checking that AbstractInt works or fails. Feel free to let me know if that's not what we want.
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.