Skip to content

[SYCL] Fix computation of the submit time based on host timestamps #12104

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

Merged
merged 5 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions sycl/source/detail/device_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -599,8 +599,8 @@ ext::oneapi::experimental::architecture device_impl::getDeviceArch() const {
return MDeviceArch;
}

// On first call this function queries for device timestamp
// along with host synchronized timestamp and stores it in memeber varaible
// On the first call this function queries for device timestamp
// along with host synchronized timestamp and stores it in member variable
// MDeviceHostBaseTime. Subsequent calls to this function would just retrieve
// the host timestamp, compute difference against the host timestamp in
// MDeviceHostBaseTime and calculate the device timestamp based on the
Expand All @@ -622,14 +622,28 @@ uint64_t device_impl::getCurrentDeviceTime() {
// To account for potential clock drift between host clock and device clock.
// The value set is arbitrary: 200 seconds
constexpr uint64_t TimeTillRefresh = 200e9;
assert(HostTime >= MDeviceHostBaseTime.second);
uint64_t Diff = HostTime - MDeviceHostBaseTime.second;

if (Diff > TimeTillRefresh || Diff <= 0) {
// If getCurrentDeviceTime is called for the first time or we have to refresh.
if (!MDeviceHostBaseTime.second || Diff > TimeTillRefresh) {
const auto &Plugin = getPlugin();
auto Result =
Plugin->call_nocheck<detail::PiApiKind::piGetDeviceAndHostTimer>(
MDevice, &MDeviceHostBaseTime.first, &MDeviceHostBaseTime.second);

// We have to remember base host timestamp right after PI call and it is
// going to be used for calculation of the device timestamp at the next
// getCurrentDeviceTime() call. We need to do it here because getPlugin()
// and piGetDeviceAndHostTimer calls may take significant amount of time,
// for example on the first call to getPlugin plugins may need to be
// initialized. If we use timestamp from the beginning of the function then
// the difference between host timestamps of the current
// getCurrentDeviceTime and the next getCurrentDeviceTime will be incorrect
// because it will include execution time of the code before we get device
// timestamp from piGetDeviceAndHostTimer.
HostTime =
duration_cast<nanoseconds>(steady_clock::now().time_since_epoch())
.count();
if (Result == PI_ERROR_INVALID_OPERATION) {
char *p = nullptr;
Plugin->call_nocheck<detail::PiApiKind::piPluginGetLastError>(&p);
Expand Down
2 changes: 1 addition & 1 deletion sycl/source/detail/device_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ class device_impl {
mutable std::once_flag MDeviceNameFlag;
mutable ext::oneapi::experimental::architecture MDeviceArch{};
mutable std::once_flag MDeviceArchFlag;
std::pair<uint64_t, uint64_t> MDeviceHostBaseTime;
std::pair<uint64_t, uint64_t> MDeviceHostBaseTime{0, 0};
}; // class device_impl

} // namespace detail
Expand Down
44 changes: 21 additions & 23 deletions sycl/test-e2e/Basic/submit_time.cpp
Original file line number Diff line number Diff line change
@@ -1,34 +1,32 @@
// RUN: %{build} -o %t.out
// RUN: %{run} %t.out
// There is an issue with reported device time for the L0 backend, works only on
// pvc for now. No such problems for other backends.
// RUN: %if (!ext_oneapi_level_zero || gpu-intel-pvc) %{ %{run} %t.out %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make it a REQUIRES instead? Seems like it would be better to just skip it. I don't think compile-only testing benefits us in E2E.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with REQUIRES at first, but for some reasons test is still executed if I do this. That's why changed to this approach.


// Check that device_impl::isGetDeviceAndHostTimerSupported() is not spoiling
// device_impl::MDeviceHostBaseTime values used for submit timestamp
// calculation.
// Check that submission time is calculated properly.

#include <sycl/sycl.hpp>

using namespace sycl;

int main(void) {
sycl::queue queue(
sycl::property_list{sycl::property::queue::enable_profiling()});
sycl::event event = queue.submit([&](sycl::handler &cgh) {
cgh.parallel_for<class set_value>(sycl::range<1>{1024},
[=](sycl::id<1> idx) {});
});
sycl::queue q({sycl::property::queue::enable_profiling{}});
int *data = malloc_host<int>(1024, q);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leaks.


// SYCL RT internally calls device_impl::isGetDeviceAndHostTimerSupported()
// to decide how to calculate "submit" timestamp - either using backend API
// call or using fallback implementation.
auto submit =
event.get_profiling_info<sycl::info::event_profiling::command_submit>();
auto start =
event.get_profiling_info<sycl::info::event_profiling::command_start>();
auto end =
event.get_profiling_info<sycl::info::event_profiling::command_end>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit; We try to discourage the use of using namespace sycl; so maybe we should remove that instead of removing the qualifications... Just to be good role models. 😉

for (int i = 0; i < 20; i++) {
auto event = q.submit([&](sycl::handler &cgh) {
cgh.parallel_for<class KernelTime>(
sycl::range<1>(1024), [=](sycl::id<1> idx) { data[idx] = idx; });
});

if (!(submit <= start) || !(start <= end))
return -1;
event.wait();
auto submit_time =
event.get_profiling_info<sycl::info::event_profiling::command_submit>();
auto start_time =
event.get_profiling_info<sycl::info::event_profiling::command_start>();
auto end_time =
event.get_profiling_info<sycl::info::event_profiling::command_end>();

assert((submit_time <= start_time) && (start_time <= end_time));
}
sycl::free(data, q);
return 0;
}