-
Notifications
You must be signed in to change notification settings - Fork 99
uniformity: test uniformity when a loop body only returns #4477
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
|
Works in dawn.node |
|
This case fails with Tint locally: Looks like the tests here do not cover the case where the is a |
Good catch. It should be covered by this rule. I've added your case locally and it fails. So that's good. I'll file a Tint issue to fix it. |
|
I want to add more cases, as found from testing the Tint fixes. See the DeadLoopTests in https://dawn-review.googlesource.com/c/dawn/+/267474 that cover |
Consider: loop { s1 continuing { s2 } }
If the statement behaviour of s1 is exactly {Return}, then the statement
behaviour of the entire loop is {Return}. In particular, nonuniformity
effects do not leak out from s2 to the context after or outside of the
whole loop.
See the WGSL spec fix for gpuweb/gpuweb#5100
Test three new statement scenarios, each where s1 starts with `return;`,
but varying where a collective operation is placed:
- immediately after the return statement
- in the continuing block of the same loop
- immediately after the loop, where the continuing block in the loop
has a break-if. This case requires the analysis to avoid using a
Next behaviour from the overall loop.
- immediately after the loop
Also test similar variants where we use `break` instead of `return`.
Also test loop constructs:
- for with a condition: desugars to loop with initial `if !(cond) { break;}`
- for without a condition: desugars to loop without that initial
conditional break.
- while loop
Fixed: gpuweb#4476
|
I've reworked the statement uniformity checks to use an explicit And I've added the variants from the Tint unit test, as described in the updated commit comment. |
|
Passes on dawn.node when Dawn is patched with https://dawn-review.googlesource.com/c/dawn/+/267474 |
Fixed: #4476
Issue: #4476
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.