Use CRS matrix sort, instead of Kokkos::sort on each row #1553
Conversation
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
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
vqd8a
approved these changes
Sep 28, 2022
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC930_Light_Tpls_GCC930
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CUDA11_CUDA11_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC1020
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC1020_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL19
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG13CUDA10
Jenkins Parameters
Using Repos:
Pull Request Author: brian-kelley |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_GCC930_Light_Tpls_GCC930
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CUDA11_CUDA11_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC1020
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC1020_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL19
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG13CUDA10
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ vqd8a lucbv ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge |
jgfouca
added a commit
to jgfouca/kokkos-kernels
that referenced
this pull request
Oct 18, 2022
* origin/develop: (177 commits) cm_test_all_sandia: fix base module list cm_test_all_sandia: updates and cleanup Remove src subdir TeamGemvInternal: Update headers and namespace in KokkosBatched_Gemv_Decl.hpp apply clang-format change to device_type scripts/cm_test_all_sandia: Update openblas tpl sparse/unit_test: Disable spmv_mv_heavy for all A64FX builds Use CRS matrix sort, instead of Kokkos::sort on each row (kokkos#1553) Avoid the SIMD code branch if the batched size is not a multiple of the vector length SYCL: Fix linking with ze_loader in Trilinos fix the failing tests cm_test_all_sandia: testing updates Fix warnings Use static_cast<double> instead of 1. * Update the batched sparse kernels to add SIMD data types and options for the temporary variable allocations. Update default argument ROCm version 4.5 -> 5.2 in Docker image Fix HIP nightly build to satisfy new Kokkos minimum requirements blas: apply clang-format-8 ...
ndellingwood
pushed a commit
to ndellingwood/kokkos-kernels
that referenced
this pull request
Jan 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
spiluk_symbolic needs to sort the rows of the L and U graphs. Instead of using Kokkos::sort on each row (since this is meant for device-wide sorts of large arrays), use the KokkosSparse::sort_crs_graph function executing on host.
This fixes trilinos/Trilinos#11041. The issue appeared to come from Kokkos because the default behavior of Kokkos::sort changed recently. In 3.6.1, given a UVM space view it dispatched to std::sort because it's host-accessible. In 3.7, In was dispatching to Kokkos bin sort on the GPU. This is definitely the way it should be in general given how Kokkos::sort is intended to be used, but it made this case of sorting many small arrays a lot slower. Especially when those small arrays live in UVM that's resident in host memory.
For biggest local matrix on abnormal energy surrogate problem (the one from Trilinos issue), here are the timings just for sorting L and U a few different ways:
Kokkos::sort each row, with force kokkos=true (current behavior): 169.452
My radix sort_crs_matrix, all on Serial/host (this PR): 3.42121
My bitonic sort_crs_matrix on V100, but not including the transfer back to host: 3.3849
std::sort each row in a normal for loop: 1.92318
This data suggests a follow-up change to sort_crs_graph, that uses std::sort instead of my radix sort where possible. That can come later though - this PR fixes the big slowdown.