-
Notifications
You must be signed in to change notification settings - Fork 99
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
fix: nested namespace holding kk mkl implementation #2100
Conversation
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
@maartenarnst sorry for the slow comment here... why not use a non qualified name instead of using this fully qualified one since we are in the same namespace? I think that would look cleaner... |
@@ -63,7 +63,7 @@ inline void mkl_internal_safe_call(sparse_status_t mkl_status, const char *name, | |||
} | |||
|
|||
#define KOKKOSKERNELS_MKL_SAFE_CALL(call) \ | |||
KokkosSparse::Impl::mkl_internal_safe_call(call, #call, __FILE__, __LINE__) | |||
::KokkosSparse::Impl::mkl_internal_safe_call(call, #call, __FILE__, __LINE__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
::KokkosSparse::Impl::mkl_internal_safe_call(call, #call, __FILE__, __LINE__) | |
mkl_internal_safe_call(call, #call, __FILE__, __LINE__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I agree that the macro is defined here in a piece of code inside the KokkosSparse::Impl
namespace. But, then, when the macro is used somewhere else, this namespace is not taken with it.
E.g. when it's used here
kokkos-kernels/sparse/src/KokkosSparse_spgemm_handle.hpp
Lines 240 to 242 in afd65f0
~mklSpgemmHandleType() { KOKKOSKERNELS_MKL_SAFE_CALL(mkl_sparse_destroy(C)); }
it's called in a piece of code in the KokkosSparse
namespace without the Impl
. So shouldn't we keep the proposed change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point this would obviously create issues. Would you mind moving the definition of the macro KOKKOSKERNELS_MKL_SAFE_CALL
to line 232, just after the namespaces are closed?
My suspicion is at that point the leading ::
in your fix might not be needed anymore?
I opened PR #2134 that fixes the issue in a way that I find cleaner, let me know if you have an issues closing this? |
@maartenarnst can you see if your issues are resolved with PR #2134 being merged now? |
This PR fixes a compilation error when compiling Trilinos/kokkos-kernels when both
mkl
androcsparse
are enabled.Compilation errors look like this:
The fix is to add a
::
so as to look forKokkosSparse::Impl
explicitly in the global namespace.