[SYCL][Graph] async_malloc use allocation size for zeVirtualMemQueryPageSize #19402
Conversation
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
lslusarczyk
force-pushed
the
size_aware_urVirtualMemGranularityGetInfo
branch
from
0a4f4c7 to
f9a4616
Compare
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
lslusarczyk
force-pushed
the
size_aware_urVirtualMemGranularityGetInfo
branch
from
f9a4616 to
4befbf0
Compare
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
lslusarczyk
force-pushed
the
size_aware_urVirtualMemGranularityGetInfo
branch
from
4befbf0 to
9475527
Compare
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| asyncAllocWorksWithSize(131); | ||
| asyncAllocWorksWithSize(10071); | ||
| asyncAllocWorksWithSize(1007177); | ||
| // asyncAllocWorksWithSize(191439360); // BUG |
There was a problem hiding this comment.
You should uncomment this and add an expected failure for L0.
There was a problem hiding this comment.
uncommented, bug is fixed, test passes
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
EwanC
changed the title
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
lslusarczyk
changed the title
| granularity_mode Mode, | ||
| size_t allocationSize) { |
There was a problem hiding this comment.
This seems to be an ABI-breaking change, we can't just do it like that.
There was a problem hiding this comment.
Thank you for pointing it out. How shall I make this change? I guess there is some way of changing ABI.
I see: sycl/doc/developer/ABIPolicyGuide.md
There was a problem hiding this comment.
This method is part of an experimental extension https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/experimental/sycl_ext_oneapi_virtual_mem.asciidoc Acording to https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ABIPolicyGuide.md Features clearly marked as experimental are considered as an exception to this guideline.
I understand although ABI change is possible I should get consensus on API change with virtual_mem extension owners. @steffenlarsen , @aelovikov-intel , @aarongreig - you are code contributors for virtual mem, should I discuss it with you, some of you, some other people too? I'd like to start email thread with interested people about this.
There was a problem hiding this comment.
I think in this case it would be as simple as using two overloads instead of default value for the allocationSize. Was previous behavior the same as = 1 case of the new impl?
There was a problem hiding this comment.
Yes. Previous behavior is the same as new one with =1.
Ok. I'm adding an additional overload. Thank you for suggestion.
There was a problem hiding this comment.
Done. Please check if the signature looks OK. If so I'll update extension documentation.
lslusarczyk
force-pushed
the
size_aware_urVirtualMemGranularityGetInfo
branch
from
482ce66 to
2b622d3
Compare
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
lslusarczyk
force-pushed
the
size_aware_urVirtualMemGranularityGetInfo
branch
from
2b622d3 to
889a26a
Compare
lslusarczyk
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
lslusarczyk
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| @@ -28,6 +28,9 @@ set(UR_BUILD_TESTS "${SYCL_UR_BUILD_TESTS}" CACHE BOOL "" FORCE) | |||
| # UR tests require the examples to be built | |||
| set(UR_BUILD_EXAMPLES "${SYCL_UR_BUILD_TESTS}" CACHE BOOL "" FORCE) | |||
|
|
|||
| option(SYCL_UR_FORMAT_CPP_STYLE "Format code style of UR C++ sources" OFF) | |||
There was a problem hiding this comment.
Why do we need this? Can't we just pass UR_FORMAT_CPP_STYLE when configuring SYCL?
In L0 we need to call zeVirtualMemQueryPageSize with the actual allocation size for the virtual/physical allocations to align correctly.
Right now we check alignment without passing any size: https://github.com/intel/llvm/blob/sycl/sycl/source/detail/graph/memory_pool.cpp#L45
This ends up translating to 1 byte in the call to L0: https://github.com/oneapi-src/unified-runtime/blob/de05f984aa19458a4993d2a2709e3b79d82f1a37/source/adapters/level_zero/virtual_mem.cpp#L32-L37 and for large allocations a wrong alignment is used and L0 reports ZE_RESULT_ERROR_UNSUPPORTED_SIZE upon zePhysicalMemCreate call (UR fails with UR_RESULT_ERROR_INVALID_VALUE then).
The UR API should change to accept a size.
This PR exposes this issue in a unittest and fixes it.