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

KokkosBlas::gemv and gemm no longer compile with mixed CudaSpace and CudaUVMSpace views. #1913

Closed
vbrunini opened this issue Jul 25, 2023 · 7 comments
Assignees

Comments

@vbrunini
Copy link

We have previously used KokkosBlas::gemv and gemm with A being a matrix in CudaSpace and x/y (or b/c for gemm) being in CudaUVMSpace. As of the most recent snapshot to Trilinos this now fails to compile due to the static_asserts that check SpaceAccessibility<...>::assignable.

@lucbv

@lucbv lucbv self-assigned this Jul 25, 2023
@lucbv
Copy link
Contributor

lucbv commented Jul 25, 2023

Thanks for reporting this @vbrunini I would have thought that CudaUVMSpace is assignable from the Cuda execution space?
I will talk with the Kokkos Core team and clarify that, in general the new assertion is there to make sure that users do not pass something that is stored in HostSpace with a device execution space.

@lucbv lucbv added the bug label Jul 25, 2023
@lucbv
Copy link
Contributor

lucbv commented Jul 25, 2023

Ah, actually the issue is with assignable vs accessible that makes sense, my bad for misunderstanding, will get that fixed.

@lucbv
Copy link
Contributor

lucbv commented Jul 25, 2023

Okay, should be addressed in PR #1914

@vbrunini
Copy link
Author

Thanks!

@lucbv
Copy link
Contributor

lucbv commented Jul 28, 2023

Can you check that everything works fine now that #1914 was merged?
If so we can close this issue.

@vbrunini
Copy link
Author

vbrunini commented Aug 1, 2023

I think the change looks good, but it will be a bit before I get a chance to test it in sierra. Could you add a kokkos-kernels test for the mixed memory space usage? I think if you do that it's safe to close this before I get a chance to test in the app.

@lucbv
Copy link
Contributor

lucbv commented Aug 1, 2023

I can add such a test but we will have to be a bit careful with it as it requires the flag KOKKOSKERNELS_TEST_ETI_ONLY=OFF and this is only tested in one of our CI builds and it is running on CPU...
I'll discuss with others to see if we can add such a build on GPU but we might simply not have the testing capacity for it, we need to check.

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

No branches or pull requests

3 participants