[SYCL] Support memset, memcmp in libdevice #4147
Conversation
jinge90
requested review from
a team,
AlexeySachkov,
kbobrovs,
mlychkov and
vzakhari
as code owners
This PR adds memset and memcmp in libdevice. Latest llvm-spirv can handle llvm.memset intrinsic, so we just rely on "__builtin_memset" which is the same way as memcpy. For memcmp, there is no support we can rely on in latest memcpy, so libdevice provides a simple implementation. Signed-off-by: gejin <ge.jin@intel.com>
|
Test for memcpy, memset, memcmp is in intel/llvm-test-suite#371 |
Signed-off-by: gejin <ge.jin@intel.com>
|
/summary:run |
|
Hi, all. |
|
@vzakhari, could you take a look, please? |
| ========================== | ||
|
|
||
| .. code: | ||
| void *__devicelib_memcpy(void *dest, const void *src, size_t n); |
There was a problem hiding this comment.
does this work for any combination of address spaces pointed to by dest and src? Might be worth adding a note. Std C library is not concerned with address spaces.
There was a problem hiding this comment.
Hi, @kbobrovs
I think no matter what combination of address spaces pointed to by dest and src is, it should work if we can "read" from src and "write" to "dest". Is this correct?
Thanks very much.
There was a problem hiding this comment.
I don't know. I would expect this to work with any combination. My point that such details should be added to the user API description.
There was a problem hiding this comment.
Actually it will only work for __generic address space pointers. Is there a way in DPC++ to specify __generic pointer explicitly?
There was a problem hiding this comment.
By default, function pointer arguments are generic. But users needs to understand if he/she can do e.g.
int arr[100]; // local variable in a function, resides in the private address space
...
auto *s1 = local_accessor.get_pointer();
auto *s2 = &arr[0];
__devicelib_memcpy(s1, s2, 100);There was a problem hiding this comment.
Hi, @kbobrovs and @vzakhari
__devicelib_memcpy is called by memcpy whose input pointer parameter is standard C/C++ pointer with address space.
So, if developer calls memcpy in SYCL device, the pointer is a generic_space pointer. And no matter where the input generic pointer points to, developers should ensure that they have access to the memory.
I also created a PR for memory function test: intel/llvm-test-suite#371 which includes some basic tests and alignment test, I will update it to add some tests covering different address space memory.
Thanks very much.
|
LGTM except for the alignment issue that Konst pointed out. |
jinge90
dismissed stale reviews from mlychkov, s-kanaev, gmlueck, and AlexeySachkov
via
ea32ef8
Signed-off-by: gejin <ge.jin@intel.com>
| ========================== | ||
|
|
||
| .. code: | ||
| void *__devicelib_memcpy(void *dest, const void *src, size_t n); |
There was a problem hiding this comment.
Actually it will only work for __generic address space pointers. Is there a way in DPC++ to specify __generic pointer explicitly?
kbobrovs
left a comment
kbobrovs
left a comment
There was a problem hiding this comment.
I think this patch is still missing couple of things:
- documenting behavior when when pointers to various address spaces are used as arguments
- tests for different address space combinations
(will be on vacation next 3 weeks, feel free to proceed w/o my approval)
Signed-off-by: gejin <ge.jin@intel.com>
jinge90
requested review from
AlexeySachkov,
gmlueck,
kbobrovs,
mlychkov and
s-kanaev
|
/summary:run |
|
/summary:run |
|
Hi, @AlexeySachkov |
Hi, @bader |
8 participants
This PR adds memset and memcmp in libdevice. Latest llvm-spirv can handle
llvm.memset intrinsic, so we just rely on "__builtin_memset" which is the
same way as memcpy. For memcmp, there is no support we can rely on in latest
llvm-spirv, so libdevice provides a simple implementation.
Signed-off-by: gejin ge.jin@intel.com