Skip to content

Don't use switch fallthrough #423

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Don't use switch fallthrough #423

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f624aa8
Don't use switch fallthrough
llvm-beanz Sep 2, 2025
706b13e
Fix comments and don't truncate buffers
llvm-beanz Sep 2, 2025
1a4c00f
Bleh
llvm-beanz Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions test/Feature/WaveOps/WaveActiveAnyTrue.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,11 @@ RWStructuredBuffer<uint> Out : register(u1);
[numthreads(4, 1, 1)]
void main(uint3 threadID : SV_DispatchThreadID) {
bool B1 = false;
switch (value[threadID.x]) {
case 0:
case 2: // threads 0, 1, 2; result for each false
Out[threadID.x] = WaveActiveAnyTrue(B1);
B1 = true;
break;
default: // thread 3; result is false
Out[threadID.x] = WaveActiveAnyTrue(B1);
break;
if (value[threadID.x] == 0 || value[threadID.x] == 2) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Out[threadID.x] = WaveActiveAnyTrue(B1);
B1 = true;
} else {
Out[threadID.x] = WaveActiveAnyTrue(B1);
}
// result for all threads is true because B1 is true for threads 0-2
Out[threadID.x + 4] = WaveActiveAnyTrue(B1);
Expand Down
16 changes: 6 additions & 10 deletions test/Feature/WaveOps/WaveActiveCountBits.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,12 @@ RWStructuredBuffer<uint> Out : register(u1);
void main(uint3 threadID : SV_DispatchThreadID) {
bool B1 = false;

switch (value[threadID.x]) {
case 0: // threads 0 and 1; result is number of active lanes (2)
Out[threadID.x + 4] = WaveActiveCountBits(true); // threads 0 and 1
case 2:
B1 = true; // set b1 to true for thread 3
break;
default:
Out[threadID.x + 4] = WaveActiveCountBits(false); // thread 2; expect 0
break;
}
if (value[threadID.x] == 0)
Out[threadID.x + 4] = WaveActiveCountBits(true); // threads 0 and 1
if (value[threadID.x] == 0 || value[threadID.x] == 1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This isn't equivalent to the original behavior. == 1 should be == 2 for equivalent behavior.

B1 = true; // set b1 to true for thread 3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sets B1 to true for threads 0, 1, and 2, but not 3, due to LUT value being [0, 0, 1, 2].

else
Out[threadID.x + 4] = WaveActiveCountBits(false); // thread 2; expect 0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// should be 3 because B1 set to true for threads 0,1, and 3.
uint Count = WaveActiveCountBits(B1);
Out[threadID.x] = Count;
Expand Down
Loading