-
Notifications
You must be signed in to change notification settings - Fork 754
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][CUDA][PI][runtime][ABI-break] Add support for multi-device context #6446
Closed
Closed
Changes from all commits
Commits
Show all changes
50 commits
Select commit
Hold shift + click to select a range
82b8bb9
cuda plugin changes
t4c1 02ceed2
modification to PI interface and runtime functions
t4c1 58591b2
Merge branch 'sycl' into multi_device_context
t4c1 6da5ea9
revert CUDA part of "[SYCL][CUDA][HIP] Report every device in its own…
t4c1 3c44f17
handle multiple contexts in program and kernel and memory transfers i…
t4c1 70c049c
fix memory transfers between devices
t4c1 7215c67
Merge branch 'sycl' into multi_device_context
t4c1 81ef2cf
enable multiple devioces in one context
t4c1 47a8200
format
t4c1 522a246
Merge branch 'sycl' into multi_device_context
t4c1 321829a
fix tests
t4c1 0bc7d62
Fix typo
joeatodd e014f8b
addressed comments
t4c1 301bd66
removed commenteed code
t4c1 5bdaef7
addressed review comments
t4c1 dbefec7
Merge branch 'sycl' into multi_device_context
t4c1 ec70ee0
format
t4c1 1d6198e
manual format
t4c1 a3761c0
fix pi_esimd_emulator
t4c1 ed57c83
actually fix pi_esimd_emulator
t4c1 791c8b5
fix for subdevice related tests
t4c1 3cb9042
format
t4c1 cd3c827
Merge branch 'sycl' into multi_device_context
t4c1 6a1bf98
format
t4c1 938bbe0
added querry for plugin memory connection
t4c1 d918d26
fix pi abi checks
t4c1 c2d1c6c
Apply suggestions to use ARG_UNUSED in pi_esimd_emulator.cpp from cod…
t4c1 7aeac7a
Merge remote-tracking branch 'origin/sycl' into HEAD
npmiller cc63a08
Fix PiMock after merging
npmiller 62e1751
Fix ABI test
npmiller c8042bd
Cleanup memory connection interface
npmiller 61385ce
Fix formatting
npmiller e54d662
Fix assert test
npmiller ac679f0
Fix ABI break for allocateMem
npmiller 09560ae
Fix device_index function
npmiller 50314ea
Refactor graph builder
npmiller 341a4b4
Revert "Refactor graph builder"
npmiller 11934a3
Merge remote-tracking branch 'origin/sycl' into multi_device_context
npmiller a1c0111
Fix merge conflicts
npmiller 76d01d5
Merge remote-tracking branch 'origin/sycl' into multi_device_context
npmiller 8f4c0ce
[SYCL][CUDA] Fix 2D USM active context
npmiller e06c597
[SYCL][CUDA] Clarify CUDA context usages
npmiller 25dd438
[SYCL][CUDA] Keep track of devices in PI allocations
npmiller 9179ee9
Merge remote-tracking branch 'origin/sycl' into multi_device_context
npmiller 7e61df9
[SYCL][CUDA] Fix merge issues
npmiller fefc782
Merge remote-tracking branch 'upstream/sycl' into multi_device_context
npmiller d69134c
Fix PI cuda tests for multi-device context
npmiller 5d4c941
Merge remote-tracking branch 'upstream/sycl' into multi_device_context
npmiller c5d6a0e
[SYCL] Update tests for multi-device context
npmiller deba394
[SYCL] Fix linting issues
npmiller File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you need to add "device"? How is it used and what if it is not given?
We didn't need that for buffer migration in L0 BE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the device, the memory will be allocated on. Even if we are putting multiple CUDA devices in a single context, they still have distinct device memories, so for the allocation we need to know what device to allocate on. It needs to be given and the runtime is changed in this PR so that it is always given. On backends that do not need this information device argument can be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffer would be represented by multiple allocations, one on each device in the context. It would then "migrate" (copy between allocations) depending on where the buffer is being accessed from. So what is that single "device" that you are adding and why it was not necessary for OpenCL and Level-Zero but is needed for CUDA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For CUDA backend we want to be able to put multiple devices in the same context even when they do not share the same memory. So memory allocation can not be located in a context (that could contain different devices with distinct memories), but must be located on a specific device.
As far as I understand it other backends so far only supported putting multiple devices in the same context when they share memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, both OpenCL and Level-Zero backends support context with multiple devices in it even if they have their distinct memories. That's why I said there multiple allocations representing such a buffer in the context, and it is being copied essentially from device where it was used the last to where it is needed next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it is a possible way to go, but I am not sure it justifies the "complexity" of breaking backwards compatibility (btw, I am still not sure why you want to have some "initial" device for a buffer allocation since that can immediately change as app starts using it from other devices). The buffer "migration" across devices in the context implemented in OpenCL and Level-Zero is not a big deal, in my view, so how do we justify a re-course?
Adding few folks for extra opinions: @steffenlarsen , @romanovvlad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no preemptive allocation on some device with this approach. You only have a device allocation when you are first using a buffer on that device. It can only migrate to another device after that allocation is first used on the device it was allocated on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is the device that first used that buffer, and nothing more? Eventually it will be migrated (by SYCL RT in your proposal) to whatever other device that needs it in this context, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. Except if the backend returns
PLUGIN_MANAGED_OR_SAME
frompiGetMemoryConnection
. In this case RT will do nothing and assume the backend can handle any migrations it needs. That can be used for any pair of devices in the same context for the OpenCL backend.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the query.