-
Notifications
You must be signed in to change notification settings - Fork 205
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 tests for cl_khr_expect_assume #1888
Conversation
Tests expect with 64-bit SPIR-V binaries.
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.
The approach looks alright to me, just a few nits. My comments on expect_char.spvasm64
apply to most other expect .spvasm files too.
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, thanks!
@karolherbst FYI, I tried these tests on rusticl and the "expect" tests are crashing. Is this something you'd like to look at before merging this PR? I think the SPIR-V files are correct and they're passing the validator, but it's possible I messed something up. Here is a backtrace. I'm using a rusticl I built myself, from the mesa-24.0.0 tag, so it's possible I messed up something as part of the build, too.
|
I think that's the mesa internal spirv parser hitting something unknown or unexpected. I can take a look shortly and see what's up. Thanks for the ping. |
The only apparently difference I see is the |
Actually.. that's returned by the runtime and the CTS just prints the result of |
@bashbaug mind sharing the output of |
@bashbaug nevermind... this is actually a regression it seems? I was debugging a different bug which ends up in the same trace and I think something broke. on main everything seems fine. Sadly it's also broken with 24.0.1, so I'll need to figure out what broke where. In any case... rusticl seems to pass the tests here (ignoring the regression) |
for reference, the issue was fixed by https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27265 |
Thanks for the quick follow-up. If everything is working on your mainline it sounds like we're good to merge this, then? |
yeah, I've submitted a MR to backport the fixes here: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27740 apparently the initial implementation wasn't entirely correct. Oh well, at least that's sorted out now. And thanks for making me aware of this. I didn't know there was a bug and the fix wasn't marked for backporting either... |
Regarding:
This is due to a bug in the CTS test reporting (already fixed) where the supported SPIR-V versions were checked from the lowest version to the highest version and the first match was printed. All should be well if I merge or rebase to pick up the latest changes. |
I'll need to add a test case checking OpExpectKHR with boolean sources to close #1424, but I'd like to do that in a different PR if everything looks good here. |
Merging as discussed in the February 27th teleconference. |
This PR adds tests for the cl_khr_expect_assume extension. Because this functionality is only exposed via SPIR-V (for now), only the spirv_new test is updated:
a. For each of the supported data types, tests scalars and vectors of 2, 3, 4, 8, and 16 components.
For reference: