[SYCL] native image handle support for LevelZero. #8603
[SYCL] native image handle support for LevelZero. #8603againull merged 25 commits intointel:syclfrom
Conversation
… interop_handle::get_native_mem. Signed-off-by: Chris Perkins <chris.perkins@intel.com>
…om/cperkinsintel/llvm into cperkins-make_image-and-interop-L0
|
Jenkins/llvm-test-suite is passing over at intel/llvm-test-suite#1649 |
gmlueck
left a comment
gmlueck
left a comment
There was a problem hiding this comment.
Note that #7196 also adds some new APIs to this extension under version 4. Assuming they both go in at about the same time (and in the same DPC++ release), I think we could combine the two changes and have them both added in version 4.
sycl/doc/extensions/supported/sycl_ext_oneapi_backend_level_zero.md
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_oneapi_backend_level_zero.md
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/supported/sycl_ext_oneapi_backend_level_zero.md
Outdated
Show resolved
Hide resolved
|
memory_consumption.cpp is a known flaky test, get_thread_id.cpp failure also known Also I need approvals on behalf of @intel/llvm-reviewers-runtime and @intel/dpcpp-esimd-reviewers . Note: this doesn't make any real changes to ESIMD/CUDA etc, it merely expands the PI interface and adds dummy placeholder routines to those plugins. |
sycl/test-e2e/Plugin/interop-level-zero-image-get-native-mem.cpp
Outdated
Show resolved
Hide resolved
| backend_return_t<Backend, image<Dims>> get_native_mem( | ||
| const detail::image_accessor<DataT, Dims, Mode, | ||
| /*access::target::image*/ Target, | ||
| IsPlh /*, PropertyListT */> &Acc) const { |
There was a problem hiding this comment.
This overload confuses me a little. detail::image_accessor is a base-class of other accessor. Why can't we just have the one above, but with the inverse enable_if condition?
There was a problem hiding this comment.
It could be done that way too. I thought this was clearer because accessor is complex. We have image_accessor but there is no buffer_accessor, so the first specialization is for any type of accessor when not using target image, and this one is for image_accessor. Meaning, if in the future we were to add a third or fourth type of accessor, this code will continue to work without needing an update.
There was a problem hiding this comment.
Meaning, if in the future we were to add a third or fourth type of accessor, this code will continue to work without needing an update.
You mean if we add more accessors variants that inherit from image_accessor? If so, I worry that this implicit behavior would be as much a detriment as suddenly we have a potentially undocumented get_native_mem variant.
As for how clear this is, I somewhat agree that using something called image_accessor is clearer (and with SYCL 2020 image accessors will be clearer to the user too), but while reading it I saw the detail namespace and my first impression was that this was an internal interface in the public space.
There was a problem hiding this comment.
I don't believe this is a concern big enough to block this PR.
|
@steffenlarsen / @dm-vodopyanov - you two are the only reviewers from llvm-reviewers-runtime. Ping . |
8 participants
New interop support for images for LevelZero. Includes make_image and interop_handle::get_native_mem.
Tests are present in this PR.