Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL] Make device comparison method to analyze impl not native handles #6880

Merged
merged 4 commits into from
Sep 29, 2022

Conversation

KseniyaTikhomirova
Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova commented Sep 26, 2022

Initial issue was sycl::link failure with many devices passed when esimd_emulator backend is enabled.
esimd_emulator does not have getNative method implemented while we checked object equality by comparison of native handles.
It seems to be not fully valid and we need to check it using operator. GetNative implementation for esimd_emulator is not needed.

Signed-off-by: Tikhomirova, Kseniya kseniya.tikhomirova@intel.com

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
@KseniyaTikhomirova KseniyaTikhomirova marked this pull request as ready for review September 27, 2022 08:47
@KseniyaTikhomirova KseniyaTikhomirova requested a review from a team as a code owner September 27, 2022 08:47
sergey-semenov
sergey-semenov previously approved these changes Sep 27, 2022
@v-klochkov
Copy link
Contributor

Could you please add a LIT test that fails without PR and passes with this PR?

@KseniyaTikhomirova
Copy link
Contributor Author

Could you please add a LIT test that fails without PR and passes with this PR?

we have test that found this issue: sycl_cts/sycl_link_different_device_for_sycl_link
although the problem is that to enable this check in open source I need to enable esimd_emulator here.

@pvchupin pvchupin requested a review from a team September 27, 2022 19:01
@v-klochkov
Copy link
Contributor

pvchupin requested a review from https://github.com/orgs/intel/teams/dpcpp-esimd-reviewers

@pvchupin - I think there is no any ESIMD specific in this patch, it is purely SYCL/RT.

I can only tell that the current fix requires more changes, IMO, because std::unique may keep duplicates.

Also, enforcing usage of device::operator== turns into lhs.impl==rhs.impl, where lhs/rhs are shared_ptr<device_impl>, i.e. it is comparison of pointers to device_impl, and I don't know if this is how devices should be compared.
Need to ask for SYCL-standard or SYCL-RT experts.

@pvchupin
Copy link
Contributor

I was thinking it involves esimd_emulator...

@KseniyaTikhomirova
Copy link
Contributor Author

pvchupin requested a review from https://github.com/orgs/intel/teams/dpcpp-esimd-reviewers

@pvchupin - I think there is no any ESIMD specific in this patch, it is purely SYCL/RT.

I can only tell that the current fix requires more changes, IMO, because std::unique may keep duplicates.

Also, enforcing usage of device::operator== turns into lhs.impl==rhs.impl, where lhs/rhs are shared_ptr<device_impl>, i.e. it is comparison of pointers to device_impl, and I don't know if this is how devices should be compared. Need to ask for SYCL-standard or SYCL-RT experts.

@v-klochkov and @pvchupin let me to share a bit more details about esimd_emulator participation and operator== validness.

  1. Why I am mentioning esimd_emulator:
    Original issue was found by test sycl_cts/sycl_link_different_device_for_sycl_link in another repository where esimd_emulator backend is enabled by default while in intel/llvm - not. The failure was caused by the logic of filtering out duplicated devices from vector of devices passed to sycl::link as parameter. Duplication check was based on the comparison of native handles. But getNative function is not implemented for esimd_emulator that causes exception and failure. So one of possible solutions was to implement this function in backend. Although I clarified if such comparison is valid and here we are moving to the second question. And also with the change of comparison logic getNative method implementation for emulator is not needed any more.
  2. I asked @gmlueck about SYCL2020 spec statement (for object comparison I am referring to 4.5.2 in spec) about native handles equality and got the reply that
    "That just says that two SYCL objects which are the same also have the same underlying native handles. There is no guarantee here precluding some other SYCL object from having the same native handle."
    "The best way to determine if two SYCL objects are the same is to simply compare them via "operator=="."
    So our understanding of object equality based on native handles is not fully valid.
    To ensure that comparison via operator== is valid lets go a bit deeper to the SYCL RT implementation.
    We keep device_impl objects in the related platform_impl object (after get_devices it is inserted to the cache MDeviceCache and can be easily returned later). So it is unique during program lifetime. I pretty sure that comparison of device_impl as implemented in device::operator== is valid.
    PlatformImpl->getOrMakeDeviceImpl(PiDevice, PlatformImpl));

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
@sergey-semenov sergey-semenov self-requested a review September 28, 2022 10:27
@sergey-semenov sergey-semenov dismissed their stale review September 28, 2022 10:41

Added new comments

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
@KseniyaTikhomirova KseniyaTikhomirova changed the title [SYCL] Make device comparison method to use operator== [SYCL] Make device comparison method to analyze impl not native handles Sep 28, 2022
sycl/source/kernel_bundle.cpp Outdated Show resolved Hide resolved
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
@pvchupin pvchupin merged commit 9baa9d9 into intel:sycl Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants