[SYCL] Add sycl::device initial implementation #176972
Conversation
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
| // SYCL 2020 A.3. Device information descriptors. | ||
| namespace info { | ||
|
|
||
| enum class partition_property : std::uint32_t { |
There was a problem hiding this comment.
Comparing to intel/llvm I changed the type of partition enums from intptr to uint.
It was aligned with openCL type:
typedef intptr_t cl_device_partition_property;
but I doubt we should keep it the same way.
| using PlatformWithDevStorageType = | ||
| std::unordered_map<ol_platform_handle_t, std::vector<ol_device_handle_t>>; | ||
| using Platform2DevContainer = | ||
| std::vector<std::pair<ol_platform_handle_t, ol_device_handle_t>>; |
There was a problem hiding this comment.
I realized that it is not correct to use map here since rehash may cause change of device order. I have redone this and corresponding code.
|
@tahonermann, @dvrogozh, @sergey-semenov, @bader, @againull, @YuriPlyakhin, @Fznamznon FYI, published for review. |
Fznamznon
requested review from
Fznamznon,
YuriPlyakhin,
againull,
bader and
tahonermann
and removed request for
tahonermann
| return isCPU(); | ||
| case (aspect::gpu): | ||
| return isGPU(); | ||
| case (aspect::accelerator): |
There was a problem hiding this comment.
What's the difference between gpu and accelerator?
There was a problem hiding this comment.
accelerator == fpga, It is present in spec so I have to cover it, but we have no plan to get it enabled in liboffload and so in libsycl
| return; | ||
|
|
||
| // MDevices reallocation is prevented to keep correct ranges in MDeviceRange | ||
| MDevices.reserve(PlatformsAndDev.size()); |
There was a problem hiding this comment.
Hm. Does MDevices supposed to be empty or it might contain something? If the latter shouldn't that be something like MDevices.reserve(MDevices.size() + PlatformsAndDev.size())?
There was a problem hiding this comment.
It must be empty. This operation is done once in the first get_platforms call. After that the data is reused and never changes.
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
|
@dvrogozh & @bader thank you for your review. I have replied to or fixed all your comments. pinging @dm-vodopyanov who agreed to participate in review for upstreaming. |
bader
left a comment
bader
left a comment
There was a problem hiding this comment.
LGTM in general.
Just a few minor nits in comments.
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
| std::string Result; | ||
| Result.resize(ExpectedSize - 1); | ||
| callAndThrow(olGetDeviceInfo, MOffloadDevice, olInfo, ExpectedSize, | ||
| Result.data()); |
There was a problem hiding this comment.
I think this is UB. olGetDeviceInfo can write ExpectedSize bytes into Result.data() but writable size in Result.data() is only ExpectedSize - 1 after resize.
There was a problem hiding this comment.
I think it shouldn't since liboffload (same as UR) counts null terminator in the size while std::string doesn't.
Although I do agree that it's confusing. I had to check why I did it this way before answering this question. So it is not obvious and must be commented or updated to the safe version with resize(ExpectedSize). I will do the second.
There was a problem hiding this comment.
Sorry, my bad, checked it, and indeed liboffload counts null terminator, and we are allowed to write null terminator at Result.data() + Result.size() (only writing anything besides null terminator is undefined behavior).
So, if we assume that liboffload guarantees that it counts null terminator (they should document it though) then we should return the original version with resize(ExpectedSize - 1).
Or if we want to stay on safer side, we should leave current version but trim after call:
if (!Result.empty() && Result.back() == '\0')
Result.pop_back();
Current version is not very good because, for example for "Intel" count() will return 6 and operator== doesn't work as expected by user.
There was a problem hiding this comment.
yeah, I haven't thought about operator.
thanks, I returned it back and added comment ae54270
regarding safer side - I think it will be pretty confusing and will mean expecting of flaky behavior of liboffload (that shouldn't happen, if it changes - that is API breaking change) if we add this.
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
|
pinging @dklochkov-emb who will review instead @dm-vodopyanov |
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
5 participants
This is part of the SYCL support upstreaming effort. The relevant RFCs can be found here:
https://discourse.llvm.org/t/rfc-add-full-support-for-the-sycl-programming-model/74080 https://discourse.llvm.org/t/rfc-sycl-runtime-upstreaming/74479
Plan for next PR:
E2E lit configs & test for get_platforms & get_devices impl
context & USM free functions impl