-
Notifications
You must be signed in to change notification settings - Fork 117
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
Fix urDeviceGetGlobalTimestampTest and urContextCreateWithNativeHandle #837
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,8 +20,15 @@ TEST_P(urContextCreateWithNativeHandleTest, Success) { | |
// and perform some query on it to verify that it works. | ||
ur_context_handle_t ctx = nullptr; | ||
ur_context_native_properties_t props{}; | ||
ASSERT_SUCCESS(urContextCreateWithNativeHandle(native_context, 1, &device, | ||
&props, &ctx)); | ||
|
||
ur_result_t result = urContextCreateWithNativeHandle(native_context, 1, | ||
&device, &props, &ctx); | ||
|
||
if (result == UR_RESULT_ERROR_INVALID_OPERATION) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not a valid error code for urContextCreateWithNativeHandle, if its not supported it should return |
||
GTEST_SKIP(); | ||
} | ||
|
||
ASSERT_SUCCESS(result); | ||
ASSERT_NE(ctx, nullptr); | ||
|
||
uint32_t n_devices = 0; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
#include <thread> | ||
#include <type_traits> | ||
#include <uur/fixtures.h> | ||
#include <uur/utils.h> | ||
|
||
// WARNING - This is the precision that is used in the OpenCL-CTS. | ||
// - We might need to modify this value per-adapter. | ||
|
@@ -102,7 +103,12 @@ TEST_F(urDeviceGetGlobalTimestampTest, SuccessSynchronizedTime) { | |
const uint64_t allowedDiff = static_cast<uint64_t>( | ||
std::min(deviceTimeDiff, hostTimeDiff) * allowedTimerError); | ||
|
||
ASSERT_LE(observedDiff, allowedDiff); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not change the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem of changing allowDiff is that there is no amount of time that is guaranteed to work. I did run the test locally many times and there is some pretty big outliers. I suspect that it might be due to context switches in the OS which can take an unpredictable amount of time. So I could change allowDiff to some huge number and the test would still not be guaranteed to pass 100% of the time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are seeing that this test is flaky also on L0 in the CI (hence #836)... |
||
// CUDA and HIP implementations are not accurate enough. | ||
ur_platform_backend_t backend = uur::GetPlatformBackend(platform); | ||
if (backend != UR_PLATFORM_BACKEND_CUDA && | ||
backend != UR_PLATFORM_BACKEND_HIP) { | ||
ASSERT_LE(observedDiff, allowedDiff); | ||
} | ||
} | ||
} | ||
|
||
|
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.
what "reasonably" means here? are we adding that because L0 or other adapter is returning synchronized timestamps with some jitter?
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 is a small wording change aimed at managing expectations of the accuracy of this entrypoint in UR. I took the wording from the OpenCL spec. OpenCL is the only adapter which has native support for an equivalent entrypoint. And, even OpenCL does not provide guarantees on the accuracy of the synchronization (hence the "reasonably" wording). I think that the UR spec should be relaxed to at least be similar in scope to OpenCL. I don't think we will ever be able to enforce hard constraints on this entrypoint for CUDA / HIP (and probably L0 as well?).
This PR is marked as draft because I'm not sure what to do with the existing CTS testing. As it stands, OpenCL is probably the only adapter that can pass this reliably.