[KHR] Prefetch host

[TApplencourt]

Replace old #280

Add prefetch_host with the new format. The text and layout are based of the prefetch code to keep consistency.

[illuhad]

LGTM!

Sidenote: AdaptiveCpp already implements this exact API; the only difference is that we don't have yet the khr_ prefix and the right feature test macro.
https://github.com/AdaptiveCpp/AdaptiveCpp/blob/develop/doc/extensions.md#acpp_ext_prefetch_host

[illuhad]

AdaptiveCpp implementation PR with interface as proposed here: AdaptiveCpp/AdaptiveCpp#1972

As a sidenote, I noticed that prefetch[_host] and mem_advise all take void* instead of const void* in the SYCL spec. It's not really clear to me why they don't operate on const void*. This of course limits the code that is legal when invoking these APIs. Is there a reason for this restriction?

[TApplencourt]

Yeah, I noticed that too. I didn't wanted to modify for this KHR but as a side note, in cuda they take a const ptr (https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__MEMORY.html#group__CUDART__MEMORY_1g856fa41c8c0d28655e37b778cb9ffc65). Same for L0 (https://oneapi-src.github.io/level-zero-spec/level-zero/latest/core/api.html#zecommandlistappendmemoryprefetch).

I guess oversight? (also should the function be const?)

[illuhad]

Yeah, I noticed that too. I didn't wanted to modify for this KHR but as a side note, in cuda they take a const ptr

That makes more sense to me. Agree that for this PR it's probably out of scope.

(also should the function be const?)

I think none of the command submission functions are const. This I can understand, since from a C++ point of view, they modify the state of handler/queue objects.

[gmlueck]

There is no such member function to queue.

[TApplencourt]

I'm ashamed : ( Good catch!

[gmlueck]

I think we should show a high level synopsis here of the additions to the handler class like:

namespace sycl {
class handler {
  void khr_prefetch_host(void* ptr, size_t numBytes);
  // ...
};
}

This is consistent with the other KHR's that add member functions:

• https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:khr-default-context-platform
• https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:khr-queue-flush-queue

[TApplencourt]

I did it but then remove it due to

• https://github.com/KhronosGroup/SYCL-Docs/blob/main/adoc/extensions/sycl_khr_queue_empty_query.adoc

Can put it back if you prefer

[gmlueck]

I think sycl_khr_queue_empty_query is the outlier. We should add the high level synopsis there too to make them all consistent.

[gmlueck]

Also here, add a high-level synopsis of the member functions added to queue.

[TApplencourt]

2 Options:

• A/ Merge this KHR
• B/ Add prefetch_host to launch_enqueue PR

---

We will ping some user so they can comment

[tdavidcl]

I would lean toward more granularity in extension and thus merge it. Which also will speedup its adoption (since its already available almost as is in AdaptiveCpp)

[nscottnichols]

As a SYCL user, I'm in favor of accepting this sooner than other larger extensions (that are in some sense more disruptive to the specification) since it is small and self contained. As others have mentioned, adding this to the specification will speed up adoption.

[gmlueck]

For those who are asking for this KHR to move forward in order to speed up adoptions, which SYCL implementation(s) are you using?

If this includes DPC++, then I wanted to explain my hesitancy. DPC++ currently provides a way to prefetch to host, but it has no effect when using the Level Zero backend because there is not yet support in Level Zero to do this prefetch. Therefore, I don't see a lot of value for DPC++ to implement a standalone "prefetch host" KHR in the short term because it would just be a no-op anyway on Level Zero. By the time we have Level Zero support, I expect that the WG will have approved #922, so I'd just as soon wait until then to have a prefetch-host KHR.

[TApplencourt]

@nscottnichols , @tdavidcl if you don't mind chiming in : ) Thanks!