[SYCL][ESIMD] Make 64 bit data use lsc version of slm_gather implementation #12595
Conversation
| @@ -3921,7 +3921,7 @@ slm_gather(simd<uint32_t, N / VS> byte_offsets, simd_mask<N / VS> mask, | |||
| static_assert(Alignment >= sizeof(T), | |||
| "slm_gather() requires at least element-size alignment"); | |||
|
|
|||
| if constexpr (VS > 1 || (!detail::isPowerOf2(N, 32) && | |||
| if constexpr (VS > 1 || (!(detail::isPowerOf2(N, 32) && sizeof(T) <= 4) && | |||
There was a problem hiding this comment.
Maybe I'm misreading this but today it looks like we will go into this if statement for sizeof(T) == 8 data, and end up callling __esimd_lsc_load_merge_slm
With this change we will go into the final else and call __esimd_gather_masked_scaled2 right? Is that correct?
If so, can you explain why we want to call this intrinsic instead of the other? The current one seems to be the LSC one which I would expect to be required for 64-bit data.
There was a problem hiding this comment.
Without the change, if you pass 64 bit data as T and no MaskedGatherScatter is available you will go to the final else where gather_impl will be called and eventually fail with assertion (old gather does not support 64 bit data out of the box) If you have MaskedGatherScatter available it will be called no matter what data type is passed. With this change if MaskedGatherScatter is not available and 64 bit data is passed then lsc version will be called that does support 64 bit data.
There was a problem hiding this comment.
The change here looks good. Please add a test case for 64-bit types to slm_gather.cpp (the one that is compiled with new LLVM IR available).
There was a problem hiding this comment.
I can't add a new test that uses 64 bit types at least for now due to test issues. I can't use slm_block_store to initialize the SLM memory due to driver issues and slm_scatter doesn't support 64 bit data. Once one of these problems is solved one way or another then I can add 64 bit data tests
There was a problem hiding this comment.
bit-cast to 32-bits + 32-bit slm_scatter can be used to initialize SLM.
There was a problem hiding this comment.
So do you think it is better to create a special code path in the test for 64 bit rather than add support for 64 bit data to gather/scatter first and then simply add the tests ?
There was a problem hiding this comment.
Ok, E2E test can wait for slm_scatter to support 8-bytes, but please create compile-time only test-case(s) in memory_properties.cpp test
There was a problem hiding this comment.
Added a test, although it is pretty much useless since memory_properties.cpp is compiled with -D__ESIMD_GATHER_SCATTER_LLVM_IR which means, no matter what data type is used new LLVM IR is used rather than old implementation
|
No longer needed as handling of 64 bit data is currently emulated without the need for lsc API |
No description provided.