[SYCL] Reduce heap allocations from sycl::local_accessor
#17147
Conversation
Ideally, there should be zero but we can't fix that without ABI break (both present, and potential future one for unanticipated future changes). Reduce what we can now: - don't allocate memory on host (no host device, APIs accessing memory must be called from inside "command") - Use `std::make_shared` to fold two allocation into one for pImpl idiom
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
sycl/source/accessor.cpp
Outdated
| @@ -119,19 +119,12 @@ const sycl::range<3> &LocalAccessorBaseHost::getSize() const { | |||
| return impl->MSize; | |||
| } | |||
| void *LocalAccessorBaseHost::getPtr() { | |||
| // Const cast this in order to call the const getPtr. | |||
| return const_cast<const LocalAccessorBaseHost *>(this)->getPtr(); | |||
| // Must not be called. | |||
There was a problem hiding this comment.
For the sake of debugging, in case this doesn't hold, could we have an assert here that always fails?
There was a problem hiding this comment.
We have error handling in the derived classes before/instead "delegating" to here.
There was a problem hiding this comment.
I'm not sure I follow. The reason I would like an assert here is to avoid us unintentionally adding code to the runtime that call these functions, be it directly or through other functions (like getQualifiedPtr().)
There was a problem hiding this comment.
Yes, adding an assertion that can't be reached costs approximately nothing and can save our bacon some time!
There was a problem hiding this comment.
asserts won't be useful, because library is built in release mode. Added abort, even though I still don't see much value here. Ideally, these shouldn't exist at all, but I can't remove them as that would remove a symbol.
There was a problem hiding this comment.
I totally agree, these should be removed ASAP, but the assert would be there to ensure us developers don't do anything we shouldn't do. It may not be a huge amount of value, but it's practically free and would make it easier for the runtime dev to understand why things are crashing over a potential nullptr-deref. I can live with abort too though.
There was a problem hiding this comment.
The abort was hit by some MKL tests, when running on Windows, but not on Linux?? See CMPLRLLVM-65833
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@rolandschulz FYI |
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
|
|
@aelovikov-intel this patch broke sycl-cts/test_accessor_legacy - https://github.com/intel/llvm/actions/runs/13580511533 |
I believe this is a bug in CTS. local/constant_buffer accessor restrict |
Initially broken in intel#17147.
Initially broken in #17147.
Ideally, there should be zero but we can't fix that without ABI break (both present, and potential future one for unanticipated future changes).
Reduce what we can now:
std::make_sharedto fold two allocation into one for pImpl idiom