[SYCL] Test queue::ext_oneapi_empty() API #1427
Conversation
againull
force-pushed
the
queue_status_tests
branch
from
bad73f4 to
4b01ee9
Compare
againull
force-pushed
the
queue_status_tests
branch
from
4b01ee9 to
7260dd5
Compare
againull
force-pushed
the
queue_status_tests
branch
from
7260dd5 to
a2bb528
Compare
|
@KseniyaTikhomirova Friendly ping. |
|
@againull hello, it has failed tests, will you fix or they are not related? |
Those testing results were expected to fail because at that moment intel/llvm PR was not merged. |
| } | ||
|
|
||
| // Wait a bit to give a chance for tasks to complete. | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); |
There was a problem hiding this comment.
could we try to avoid sleeps in tests? we could not rely on such approach.
what if to add host_task as the last task that would signal cv, atomic or lock/unlock mutex and simply wait for it instead of sleep, will it help?
There was a problem hiding this comment.
what if to add host_task as the last task that would signal cv, atomic or lock/unlock mutex and simply wait for it instead of sleep, will it help?
Unfortunately, as far as I know SYCL doesn't guarantee that such waiting will ever complete. User must perform synchronization to guarantee completion. I mean it is not guaranteed that queue will be flushed and we may just wait forever.
There was a problem hiding this comment.
@KseniyaTikhomirova, unfortunately, I don't have good ideas how to get rid of sleep right now for the reasons I described above. I can probably replace it with some loop - try several times to see if queue is empty and give up if it is not empty - but I'm not sure whether it is better or not.
There was a problem hiding this comment.
ok, unfortunately I also do not have better ideas about how to get rid of sleep call. If we want to check that the right value could be returned even without wait call - may be it could be done with unittest. Although I do not want to block you here.
There was a problem hiding this comment.
Thank you! Unittest may be a good idea.
SYCL/Basic/in_order_queue_status.cpp
Outdated
| } catch (sycl::exception &E) { | ||
| std::cout << E.what() << std::endl; | ||
| // Feature is not supported for OpenCL. | ||
| assert(Q2.get_device().get_backend() == backend::opencl); |
There was a problem hiding this comment.
I am a bit concerned about this check. It allows to check that "if exception happens - it happens with device on opencl backend" but it doesn't check that "if device is on opencl backend - exception happens". Not sure if I explain the difference properly but I want to suggest to add condition that if device is on opencl backend - exception must be observed and fail test otherwise.
…tus_tests [SYCL] Test queue::ext_oneapi_empty() API (intel#1427)
Tests for PR: intel/llvm#7583
ext_oneapi_empty() is supposed to return true if all submitted commands in the queue are completed, false is returned otherwise.