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: Use in-order queues in InterOp tests #6246

Merged
merged 5 commits into from
Jul 5, 2023

Conversation

masterleinad
Copy link
Contributor

Follow-up to #6189. Since we might be relying on sycl::queues to be in-order now, we should also use in-order queues in the interoperability tests to be on the safe side.

@masterleinad masterleinad requested a review from nliber June 28, 2023 17:16
@crtrott
Copy link
Member

crtrott commented Jun 29, 2023

Retest this please.

Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Should that not be a precondition on SYCL constructor that accept a queue?
By that it means, why aren't we checking that the user-provided queue has the "in order" property?

@masterleinad
Copy link
Contributor Author

By that it means, why aren't we checking that the user-provided queue has the "in order" property?

Good idea. 55a5899

@crtrott
Copy link
Member

crtrott commented Jun 30, 2023

We could make the things which are protected by in-order macro protected by a runtime check instead. Thus it would work either way for interop purposes. We could also wait with that until someone asks and until then we unconditionally check.

Co-authored-by: Damien L-G <dalg24+github@gmail.com>
@dalg24
Copy link
Member

dalg24 commented Jul 3, 2023

SYCL tests do not pass (precondition is in-order queue violations)

@masterleinad
Copy link
Contributor Author

SYCL tests do not pass (precondition is in-order queue violations)

Sorry about that. Fixed!

@masterleinad
Copy link
Contributor Author

For the OpenMPTarget CI build we have

 1 - Kokkos_CoreUnitTest_Serial1 (Timeout)
 4 - Kokkos_CoreUnitTest_OpenMPTarget (Timeout)
18 - Kokkos_CoreUnitTest_StackTraceTest (Timeout)

which looks unrelated.

@dalg24 dalg24 merged commit 6a8488d into kokkos:develop Jul 5, 2023
27 of 28 checks passed
@dalg24
Copy link
Member

dalg24 commented Jul 5, 2023

Please make sure you mention the enforcing the precondition in the changelog

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.

None yet

3 participants