-
Notifications
You must be signed in to change notification settings - Fork 18
Add WaveReadLaneAt
tests
#349
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
Add WaveReadLaneAt
tests
#349
Conversation
- we only want to disable the minimal set of tests
test/WaveOps/WaveReadLaneAt.64.test
Outdated
... | ||
#--- end | ||
|
||
# REQUIRES: Double, Int64 |
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.
@bogner, I know we were talking about needing to double up testing, but I'm curious for your thoughts on this one.
Combining double and int64 in the same test means we don't run this on devices that only support one or the other. While I think int64 is fairly universal at this point because 64-bit pointer math kinda requires it, there are a number of mobile architectures that don't support double (including Apple GPUs).
Does it make sense to combine 64-bit types or should we split them?
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.
Yeah, splitting these makes sense. I suspect as a rule of thumb any time we're REQUIRE
ing two different features in these tests we should probably split them up for maximum test coverage.
int NegValue = WaveReadLaneAt(InInt[TID.x], NegShiftIndex); | ||
OutShift[TID.x] = PosValue + NegValue; | ||
|
||
uint MixIndex = 0; |
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.
Should we also have a test where WaveReadLaneAt is called in one or more switch statement cases?
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.
We should be careful using switch
statements in tests. They should be fine as long as no case labels have fall through. Even trivial fall through is UB in SPIRV.
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.
LGTM, just if we should split up the testcase as mentioned
@@ -0,0 +1,181 @@ | |||
#--- source.hlsl |
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.
Should there be bool tests for WaveReadLaneAt
? I see that it supports bool in the HLSL headers
This PR implements testing for the WaveReadLaneAt intrinsic. It's an improved copy of @inbelic's work here: #295
Resolves: #144