-
Notifications
You must be signed in to change notification settings - Fork 25
[NFC] Simplify WaveActiveMax test #347
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
This was a test I had written to cover some edge cases, and the test is a bit awkward and buggy. This corrects the test to hopefully be more consistent across vendors.
|
is I know right now this is just for Metal, and for this particular case the DirectX driver matches spirv/vulkan, but are their Vulkan cases we should be considering for |
bogner
left a comment
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.
I'm not convinced adding a whole extra configuration here is worth it, so I would suggest dropping the EDGE_CONDITIONS cmake variable. More ways to test just means we'll have a harder time convincing ourselves we have proper coverage.
The test simplifications look good to me though - I suspect avoiding overwriting the same values in multiple threads will make this test a lot less flaky.
| @@ -0,0 +1,2 @@ | |||
| if "EdgeConditions" in config.available_features: | |||
| config.unsupported = True | |||
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.
This logic is exactly backwards
This was a test I had written to cover some edge cases, and the test is a bit awkward and buggy. This corrects the test to hopefully be more consistent across vendors.