Skip to content

Conversation

@veselypeta
Copy link
Contributor

@veselypeta veselypeta commented Oct 26, 2023

This PR fixes the entry-point urDeviceGetGlobalTimestamp:

  • Initialize the global EvBase event when getting platforms
  • Re-order fetching host/device times
    • Fetching the device time takes longer so fetching this first results in a more synchronized time between host/device time.

llvm-testing: intel/llvm#11669

@veselypeta veselypeta requested review from a team as code owners October 26, 2023 11:16
@fabiomestre
Copy link
Contributor

fabiomestre commented Oct 26, 2023

For sanity check, do you mind running the SuccessSynchronizedTime test in a loop 1000 times or so?

On CUDA (at least on my machine), this test was unstable (sometimes passes and sometimes fails with a large variance of the result). I struggle to understand how moving the hostTimestamp code solves the issue. Maybe it is the fact that cuEventSynchronize is called right away after cuEventRecord?

@veselypeta
Copy link
Contributor Author

For sanity check, do you mind running the SuccessSynchronizedTime test in a loop 1000 times or so?

On CUDA (at least on my machine), this test was unstable (sometimes passes and sometimes fails with a large variance of the result). I struggle to understand how moving the hostTimestamp code solves the issue. Maybe it is the fact that cuEventSynchronize is called right away after cuEventRecord?

Yep you're right this test does fail sometimes after multiple repeated calls, so I have made it optional again. However, the reason why we take the host time last, is that to fetch the device time takes for time that for host time. If we get the host-time first, then we have to wait for hip to sync the streams and calculate the time, by which time the host clock is already old.

@fabiomestre
Copy link
Contributor

fabiomestre commented Oct 26, 2023

For sanity check, do you mind running the SuccessSynchronizedTime test in a loop 1000 times or so?
On CUDA (at least on my machine), this test was unstable (sometimes passes and sometimes fails with a large variance of the result). I struggle to understand how moving the hostTimestamp code solves the issue. Maybe it is the fact that cuEventSynchronize is called right away after cuEventRecord?

Yep you're right this test does fail sometimes after multiple repeated calls, so I have made it optional again. However, the reason why we take the host time last, is that to fetch the device time takes for time that for host time. If we get the host-time first, then we have to wait for hip to sync the streams and calculate the time, by which time the host clock is already old.

Yeah, I think it makes sense for the time to be a bit more synchronized this way. It just didn't completely justify the variance that I saw on CUDA. I think there is something else going on that makes this entrypoint unreliable. My guess is that it's related to the operating system.

Anyway, this looks good to me now 👍

@npmiller
Copy link
Contributor

Note that for CUDA we moved the EvBase to the device instead, I'm not 100% sure if HIP behaves the same but it could make sense to mirror that change for HIP as well, and it might also address this issue:

@veselypeta
Copy link
Contributor Author

Note that for CUDA we moved the EvBase to the device instead, I'm not 100% sure if HIP behaves the same but it could make sense to mirror that change for HIP as well, and it might also address this issue:

* [[SYCL][CUDA] Move base event into the device intel/llvm#8312](https://github.com/intel/llvm/pull/8312)

Thanks @npmiller - I won't make this change as part of this PR, but I've created #1007 to make the change

@veselypeta veselypeta added ready to merge Added to PR's which are ready to merge and removed ready to merge Added to PR's which are ready to merge labels Oct 30, 2023
@veselypeta
Copy link
Contributor Author

@oneapi-src/unified-runtime-hip-write can you please review?

@kbenzie kbenzie added the conformance Conformance test suite issues. label Nov 2, 2023
@kbenzie kbenzie mentioned this pull request Nov 2, 2023
@veselypeta
Copy link
Contributor Author

Covered in #1030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conformance Conformance test suite issues.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants