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

Make in-order queues the default via macro #6189

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

masterleinad
Copy link
Contributor

@masterleinad masterleinad commented Jun 6, 2023

This pull request makes in-order queues the default but retains the option for us to change this later easily.
If we are using in-order queues, we don't need any of the depends_on or submit_barrier calls and for benchmarks up to moderate sizes, I see zeCommandListAppendBarrier dominating without this patch.
Related to #6035.

This pull request is in some way reverting back to in-order queues after we lifted the requirement in #5065 (but also guarding a bunch of other synchronization points necessary for out-of-order queues). Thus, this is for performance, not correctness (which was the issue before).

@masterleinad masterleinad marked this pull request as ready for review June 7, 2023 20:48
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.

Make it more obvious in the description that this is in some sort reverting back to using in-order queues. Please add a reference to the previous work (PR link) that had lifted the requirement.

core/src/decl/Kokkos_Declare_SYCL.hpp Outdated Show resolved Hide resolved
core/src/setup/Kokkos_Setup_SYCL.hpp Outdated Show resolved Hide resolved
core/src/SYCL/Kokkos_SYCL_Instance.cpp Show resolved Hide resolved
@masterleinad
Copy link
Contributor Author

Make it more obvious in the description that this is in some sort reverting back to using in-order queues. Please add a reference to the previous work (PR link) that had lifted the requirement.

I add some text in the pull request description.

@masterleinad masterleinad requested a review from dalg24 June 8, 2023 13:13
@dalg24
Copy link
Member

dalg24 commented Jun 9, 2023

Will ignore HIP timeouts. Everything else passed on Jenkins.

@masterleinad
Copy link
Contributor Author

I just collapsed all the commits into one.

@dalg24 dalg24 merged commit 80dd6ad into kokkos:develop Jun 20, 2023
28 checks passed
@jedbrown
Copy link

Hi 👋. I see 4.1 (https://github.com/kokkos/kokkos/releases/tag/4.1.00) was released without this PR. This continues to be a substantial performance regression for vector operations in PETSc. Was that intentional and is there a time when in-order queues will go in a release?

@PhilMiller
Copy link
Contributor

This was merged to develop after the branch to stabilize the 4.1 release was prepared. It will definitely be in the 4.2 release (barring problems that lead to reversion), and could potentially be included in a hypothetical 4.1.1 release if the argument for it is strong enough.

@jedbrown
Copy link

We'll be asking PETSc users to --download-kokkos --download-kokkos-commit=origin/develop (and for KK) if they care about performance, but we'd rather use a release and then users can have simpler version management (especially if they want to install Kokkos themselves instead of via --download-kokkos).

@crtrott
Copy link
Member

crtrott commented Jun 30, 2023

We could make this part of a 4.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants