Skip to content

[SYCL] Host device & queue removal (internal part, not breaking ABI) #14370

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

Merged
merged 59 commits into from
Jul 2, 2024

Conversation

KseniyaTikhomirova
Copy link
Contributor

@KseniyaTikhomirova KseniyaTikhomirova commented Jul 1, 2024

Host device support is deprecated long time ago. Although our internal host task & host accessor implementation was still using it.
This change eliminates it and remove possibility to create host queue/context and device. This brings the following changes:

  • Commands & Events could not guarantee queried Context != nullptr and Queue!= nullptr since for host task stuff no device queue/context is involved. For host task we have submitted queue instance stored in event to be able to report exceptions to user and to be able to properly handle dependencies. Submitted queue for host task is guaranteed to be not null.
  • Connection command for cross context dependencies is now attached to the queue of new command (dependency for which is being analyzed). Previously it was also related to host queue only. No perf impact is expected.
  • Stream flush command is now submitted to the same queue as corresponding kernel (previously it was submitted to the host queue). This could bring negative perf impact for stream usage with in-order queue but stream is not perf oriented feature.
    ABI breaking changes to remove is_host methods and some SYCL_EXTERN stuff will be submitted separately.

Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
@KseniyaTikhomirova
Copy link
Contributor Author

original PR was #14027 but I made a mistake during sycl branch merge caused addition of sycl commits to PR and requesting corresponding reviewers not related to my changes.

@KseniyaTikhomirova
Copy link
Contributor Author

KseniyaTikhomirova commented Jul 1, 2024

@isaacault hi, may I ask you to approve it again? I spoiled my original PR #14027 (you approved it) and opened this one. Thank you.

@bader
Copy link
Contributor

bader commented Jul 1, 2024

original PR was #14027 but I made a mistake during sycl branch merge caused addition of sycl commits to PR and requesting corresponding reviewers not related to my changes.

@KseniyaTikhomirova, I can help you to fix #14370. Ping me if you are interested.

Copy link
Contributor

@isaacault isaacault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sycl/source/detail/bindless_images.cpp LGTM.

@isaacault
Copy link
Contributor

CI failure seems related to #14248. I'm not sure what the fix is, but I've had luck working around this by frequently merging sycl branch until a successful CI run.

@KseniyaTikhomirova
Copy link
Contributor Author

@intel/llvm-gatekeepers hi, Win E2E testing failed at the last step - infrastructure issue. could we merge this PR despite on this?
CI Win result is green itself:

Total Discovered Tests: 2091
Skipped : 1959 (93.69%)
Unsupported: 65 (3.11%)
Passed : 67 (3.20%)

@steffenlarsen
Copy link
Contributor

Windows failure is after tests run and is infrastructural. Merging this.

@steffenlarsen steffenlarsen merged commit 3dc75a7 into intel:sycl Jul 2, 2024
13 of 14 checks passed
@KseniyaTikhomirova
Copy link
Contributor Author

Monitoring post-commit shows MacOS build failure with unused var in program_impl.
I will prepare PR to fix but merging #14368 is preferable - it completely removes program_impl.* files.

@KseniyaTikhomirova
Copy link
Contributor Author

post commit failures fix #14385 and #14389

@KseniyaTikhomirova
Copy link
Contributor Author

more fixes #14396

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.

5 participants