-
Notifications
You must be signed in to change notification settings - Fork 125
[L0] Return success on enable/disable p2p. #1786
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
Conversation
Signed-off-by: JackAKirk <[email protected]>
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.
LGTM, can you link your accompanying intel/llvm PR showing the e2e test results?
All CI failures are unrelated. The E2E tests that failed are known to be broken in our system (I'll add a suppress).
|
The intel/llvm CI only has a single gpu, therefore these results return early. They have to be tested locally. |
Signed-off-by: JackAKirk <[email protected]>
Apparently UR ci has more devices so the ur test should run, but I realized the macro strings were missing. I also forgot to add it for hip in #1369 so I've added the string for hip in this PR too. |
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.
LGTM for level zero
#1429 enabled the queries for p2p, but didn't correspondingly update the enable/disable functions.
This means that the portable P2P workflows, as exampled in the dpc++ tests, e.g. https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/USM/P2P/p2p_access.cpp, are broken on L0 devices.
This fixes that by implementing these functions as a no-op and returning
UR_RESULT_SUCCESS
. This is the expected behaviour since L0 enables p2p on all devices within a platform by default (see e.g. discussions here intel/llvm#6104).