Don't use switch fallthrough #423
Conversation
In SPIRV even trivial switch fallthrough is undefined behavior, so we shouldn't rely on it in wave operations tests.
| default: // thread 3; result is false | ||
| Out[threadID.x] = WaveActiveAnyTrue(B1); | ||
| break; | ||
| if (value[threadID.x] == 0 || value[threadID.x] == 2) { |
There was a problem hiding this comment.
While this matches the behavior of the replaced switch, the comment seemed to imply that thread 1 was meant to also go through this branch. The later comment also seems to assume this ("result for all threads is true because B1 is true for threads 0-2"). However thread numbers are translated through the LUT value, which is [0, 0, 1, 2].
Even accounting for this looked up value, the removed and remaining comments don't seem to align with the test's design and expected behavior. The test seems to be written in a way to make it a bit confusing on purpose.
I don't understand the intention behind these odd details of the test. Was this a mistake in the code or in the comments?
It would be nice to have an explanatory comment for the test design.
The second test has a similar issue.
| if (value[threadID.x] == 0) | ||
| Out[threadID.x + 4] = WaveActiveCountBits(true); // threads 0 and 1 | ||
| if (value[threadID.x] == 0 || value[threadID.x] == 1) | ||
| B1 = true; // set b1 to true for thread 3 |
There was a problem hiding this comment.
This sets B1 to true for threads 0, 1, and 2, but not 3, due to LUT value being [0, 0, 1, 2].
| if (value[threadID.x] == 0 || value[threadID.x] == 1) | ||
| B1 = true; // set b1 to true for thread 3 | ||
| else | ||
| Out[threadID.x + 4] = WaveActiveCountBits(false); // thread 2; expect 0 |
There was a problem hiding this comment.
This writes output for thread 3, since it's the only thread not going through the main if branch.
Incidentally, the expected outputs are also truncated, excluding this output, while it includes an expected 0 output for thread 2, which was never written from the shader (which passes due to zero-initialization of output buffer).
|
@damyanp said:
Trivial fall-through was supposed to be legal and well-defined in HLSL. Non-trivial fall-through was always supposed to be illegal in HLSL, but was never blocked by DXC, unfortunately. |
We do have an issue for the SPIRV backend to handle the underlying UB: |
In SPIRV even trivial fallthrough is UB. So we kinda have a problem until we fix it. |
| } | ||
| if (value[threadID.x] == 0) | ||
| Out[threadID.x + 4] = WaveActiveCountBits(true); // threads 0 and 1 | ||
| if (value[threadID.x] == 0 || value[threadID.x] == 1) |
There was a problem hiding this comment.
Note: This isn't equivalent to the original behavior. == 1 should be == 2 for equivalent behavior.
There was a problem hiding this comment.
In test/Feature/WaveOps/WaveActiveCountBits.test, my comments about the comments being different than behavior would be fixed with a fix to the second if condition, from:
if (value[threadID.x] == 0 || value[threadID.x] == 1)to:
if (value[threadID.x] == 0 || value[threadID.x] == 2)Which would match the original shader.
I see changes to address my other comments, but they aren't quite right now either. For instance, // thread 0-3 set b1 to true. and // thread 4 aren't right. Probably better to restore original comments and fix the above condition.
In SPIRV even trivial switch fallthrough is undefined behavior, so we shouldn't rely on it in wave operations tests.