KokkosKernels: restore size_t as default offset #13368
Merged
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. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
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: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
@brian-kelley is this change going to also be applied to kokkos-kernels, or is this just temporary in Trilinos (I want to avoid this getting clobbered by a future release) |
|
@ndellingwood The thing is, offset=int is probably the default we want for non-Trilinos users, so reverting completely in KK develop is not ideal. I guess we can detect a Tribits build and set the default based on that. @lucbv what do you think? |
Partially revert the change in 4.4.0 release that made int the default offset type instead of size_t. This restores size_t as the default for Trilinos/Tribits builds only. This is the same patch as KK 2313.
brian-kelley
force-pushed
the
SizeTOffsetDefault
branch
from
eb9ff67 to
ab7159b
Compare
|
@ndellingwood I agree that we shouldn't let this get out of sync between KK develop and Trilinos. This is now an identical patch to kokkos/kokkos-kernels#2313. I tested the change locally in both a KK standalone build and a Trilinos build. |
|
@brian-kelley for spack builds of trilinos, kokkos is now built separately outside of trilinos. I'm not sure about kokkos-kernels (maybe @bartlettroscoe knows?). If kokkos-kernels is built outside of Trilinos, will this cause issues? |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: brian-kelley |
|
@rppawlo Thanks for pointing that out. Fortunately it looks like only kokkos (core) is a dependency of trilinos, so kokkos-kernels will get built as a Trilinos package. |
@rppawlo, the official version of the Trilinos Spack package builds KokkosKernels as part of Trilinos as you can see at: at: showing But there is no indication that this Trilinos Spack package depends on the Spack-built 'kokkos-kernels' package. (If my memory is right, when I asked the Trilinos Spack integration lead about updating the official Trilinos Spack pacakge to depend on the Spack-built kokkos-kernels package, there was not much interest in doing that.) However, I think the Alegra custom Trilinos Spack package does depend on the Spack-built 'kokkos-kernels' package. I think that was the origin of: ? Not clear. |
|
@bartlettroscoe @rppawlo Ah, there's no issue if using the kokkos-kernels spack package anyway. It enables both int and size_t by default. This means that Trilinos build times using that were not affected by the 4.4 change, nor will be affected by this PR. |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
ndellingwood
approved these changes
Aug 21, 2024
There was a problem hiding this comment.
Thanks @brian-kelley , patch match approved and merged to kokkos-kernels kokkos/kokkos-kernels#2313
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ ndellingwood ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
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.
Revert the change in 4.4.0 release (kokkos/kokkos-kernels#2140) that made int the default offset type instead of size_t. This is because Tpetra still uses size_t for CrsGraph and CrsMatrix local data structures.
This change is only about speeding up compilation time; instantiating for offset=size_t means that Tpetra can actually use the functions compiled into the KokkosKernels library. Tpetra's special codepaths in
CrsMatrix::apply,BsrMatrix::applyandMatrixMatrix::multiplyusing offset=int will still work.@trilinos/kokkos-kernels
Motivation
Speed up compilation times. Right now, we are instantiating KokkosKernels for types that the Tpetra stack doesn't use (except for the few exceptions above). For the types that Tpetra does use, we are instantiating the same kernels again implicitly. Some commonly used kernels might be implicitly instantiated many times.