Execution tests for subgroupAny and subgroupAll #3924
Conversation
| - got: ${res}`); | ||
| } | ||
| } else { | ||
| if (res !== 999) { |
There was a problem hiding this comment.
This 999 is mysterious. I see it's the default value in the output buffer, as set by runComputeTest.
The 999 should be a defined constant exported from subgroup_util.ts.
| }`; | ||
|
|
||
| const prng = new PRNG(t.params.case); | ||
| // Case 0 is all 0s. |
There was a problem hiding this comment.
Yeah, thanks for putting the two special cases in. It's important!
| const prng = new PRNG(t.params.case); | ||
| // Case 0 is all 0s. | ||
| // Case 1 is all 1s. | ||
| // Other cases are filled with random 0s and 1s. |
There was a problem hiding this comment.
It's good to have a few random ones.
But this allows an incorrect but simple implementation to nearly always pass this test: Look at the first half of the bits only.
How about:
increase kNumCases to 15
a decl just before inputData to pick out which bit to set to 0.
let dropout = prng.uniformInt(32);
/// Cases 2 to 9 are many 1 bits except for a single randomly selected 0 (in position 0..31)
/// Other cases are filled with random 0s and 1s.
then inside the iterRange:
...
else if (t.params.case < 10) {
return x === dropout ? 0 : 1;
} else {
return prng.uniformInt(2);
}
There was a problem hiding this comment.
Also, I recommend making a helper function to generate the input data, since the same logic is used twice and is (going to get) pretty complicated.
There was a problem hiding this comment.
Good idea. I think this has expoed a problem with the fragment tests.
| @builtin(position) pos : vec4f, | ||
| ) -> @location(0) vec2u { | ||
| var subgroup_id = 0u; | ||
| if subgroupElect() { |
There was a problem hiding this comment.
I recommend avoiding divergence here.
The subgroup representative IDs don't have to be sequential. So it's ok to delete the if condition; it's ok for every invocation to bump the counter. The broadcast on line 372 still sends the representative ID to all the invocations in the subgroup.
I've tried a little, but it gets tricky quickly. Compilers collapsing conditional code can easily lead to too many active invocations as was shown in the experimental reconvergence tests. So for now I'm not planning on adding those tests and relying on the compute versions. |
ok, sounds good. Let's make sure this is tracked somehow: an .unimplemented() test seems best so it has a chance of getting revisited in the future. |
| const prng = new PRNG(t.params.case); | ||
| // Case 0 is all 0s. | ||
| // Case 1 is all 1s. | ||
| // Other cases are filled with random 0s and 1s. |
There was a problem hiding this comment.
Good idea. I think this has expoed a problem with the fragment tests.
|
Decided to pull the fragment tests for now. |
* Compute tests with all active invocation and partially active invocations * Fragment tests with all active invocations * Removed unimplemented dynamically uniform subgroupBroadcast test (due to const requirement)
* Remove fragment tests for now * Add helpers to generate input data and add new class of cases * Increase number of cases to 15 per variants * Export data sentinel value
alan-baker
force-pushed
the
subgroup-any-all
branch
from
38589ef to
833f9c5
Compare
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.