[SYCL] Fix error handling for max_num_work_groups launch query test #15508
Conversation
GeorgeWeb
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
GeorgeWeb
force-pushed
the
georgi/fix-max-num-groups-test-return
branch
from
8f53ea5 to
35f7f19
Compare
GeorgeWeb
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
GeorgeWeb
force-pushed
the
georgi/fix-max-num-groups-test-return
branch
from
35f7f19 to
b52d752
Compare
GeorgeWeb
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
GeorgeWeb
force-pushed
the
georgi/fix-max-num-groups-test-return
branch
from
b52d752 to
25f53e3
Compare
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
GeorgeWeb
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
GeorgeWeb
changed the title
GeorgeWeb
force-pushed
the
georgi/fix-max-num-groups-test-return
branch
from
25f53e3 to
8a183d0
Compare
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
friendly ping @steffenlarsen @intel/llvm-reviewers-runtime |
| std::cerr << "Test (LocalMemory) with exceeded resource limits failed\n"; | ||
| else | ||
| std::cerr << "Test with exceed resource limits failed\n"; | ||
| return 1; // We shouldn't be here, exception is expected |
There was a problem hiding this comment.
Alternatively, you could change the return after the catch block to 1. With this, it is now dead code.
There was a problem hiding this comment.
Sure 👍. Addressed in a new commit.
Apologies for the force-push, it was because I rebased on latest sycl branch and messed my branch updating strategy here with what I normally do in unified-runtime where I rebase on top rather the merge.
GeorgeWeb
force-pushed
the
georgi/fix-max-num-groups-test-return
branch
from
8a183d0 to
8b938cf
Compare
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
GeorgeWeb
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| @@ -198,19 +207,20 @@ int test_max_num_work_groups(sycl::queue &q, const sycl::device &dev) { | |||
| return 1; | |||
| } | |||
|
|
|||
| return 0; | |||
| return result; | |||
There was a problem hiding this comment.
I'm not sure I understand the need for the result variable. In what case would we reach this and it not being 1?
|
The AND/OR part was fixed in #16610 so I'm not sure if this PR is still needed, but if it is would you mind rebasing it @GeorgeWeb ? it seems pretty close to being merged. |
@npmiller Thanks for the update! ... and apologies for slightly abandoning this one. It is not strictly needed as that was the main reason I originally opened it up. The rest are a few minor quality improvements. I am okay to close it if you think that's fine. |
The error capturing logic escaped me when first submitting the
max_num_work_groupsimplementation and the tests alongside it. We want to bitwise OR the return resulting code from the test function (either 0 or 1) with 0 rather than AND-ing it.Previously the test was still valid for what its main purpose was, which is the assertion of the value returned from the
max_num_work_groupskernel-queue-specific launch query.This fix is to better catch the negative test of launching a kernel with invalid launch configuration in the case where the query reports that no (0) workgroups can be launched on the device for that configuration (e.g. nd_range).