Add prefixes to macros #571
Conversation
CMakeLists.txt
Outdated
| @@ -294,8 +294,8 @@ else() | |||
| endif() | |||
|
|
|||
| if(DEFINED REF_BLAS_ROOT) | |||
| find_file(REF_BLAS_LIBNAME NAMES blas.dll libblas.so HINTS ${REF_BLAS_ROOT} PATH_SUFFIXES lib lib64) | |||
| find_file(REF_CBLAS_LIBNAME NAMES cblas.dll libcblas.so HINTS ${REF_BLAS_ROOT} PATH_SUFFIXES lib lib64) | |||
| find_file(ONEAPI_ONEMKL_REF_BLAS_LIBNAME NAMES blas.dll libblas.so HINTS ${REF_BLAS_ROOT} PATH_SUFFIXES lib lib64) | |||
There was a problem hiding this comment.
Overall the change looks okay to me, my only concern is about the macro prefix choice. ONEAPI_ONEMKL is a little bit redundant and heavy in my opinion, could we consider using only ONEMKL or follow the namespace with ONEAPI_MKL (this one would have my preference)?
There was a problem hiding this comment.
I'm not sure we could use ONEAPI_MKL as I vaguely remember we had to remove occurrences of "MKL" in the past. The exceptions are the name of the backends MKLCPU, MKLGPU and the mkl namespace.
I don't mind too much between ONEAPI_ONEMKL or ONEMKL.
There was a problem hiding this comment.
Thank you for your review @lhuot.
I agree it is quite verbose. In my opinion, the best option is to use ONEMKL for now, so that it can be renamed later in ONEMATH. What do you think?
There was a problem hiding this comment.
That's fine by me. @mkrainiuk any concern with the macro prefix that should be used?
There was a problem hiding this comment.
I also support shorter name if possible (longer macro name increase the chance of a typo), ONEMKL looks great to me while we are migrating to oneMath, but does it make sense to start changing it to ONEMATH already to reduce users efforts for migrating to new macro names in their build configurations?
There was a problem hiding this comment.
thank you for the feedback @mkrainiuk,
I don't like the idea of having these variables already renamed while everything else isn't. I know @Rbiessy is working on it, but I don't want to have this period with different names in between his renaming patch and this one. Moreover, these variables change should not affect users but only internal headers. So I would rather use ONEMKL for now.
There was a problem hiding this comment.
I see, it's fine to me as long as it doesn't affect the users.
There was a problem hiding this comment.
I changed names in 2a739a8
src/config.hpp.in
Outdated
| #cmakedefine ONEAPI_ONEMKL_ENABLE_ROCSOLVER_BACKEND | ||
| #cmakedefine ONEAPI_ONEMKL_BUILD_SHARED_LIBS | ||
| #cmakedefine ONEAPI_ONEMKL_REF_BLAS_LIBNAME "@ONEAPI_ONEMKL_REF_BLAS_LIBNAME@" | ||
| #cmakedefine ONEAPI_ONEMKL_REF_CBLAS_LIBNAME "@ONEAPI_ONEMKL_REF_CBLAS_LIBNAME@" |
There was a problem hiding this comment.
These two can be removed now
There was a problem hiding this comment.
Addressed while rebasing to include this change
This commit update the macros used internally adding the prefix ONEAPI_ONEMKL_. This change creates a mismatch between the cmake variables used for configuration and the internal macros. Cmake configuration stays the same while, macros are now conformant with C++ guidelines. Signed-off-by: nscipione <nicolo.scipione@codeplay.com>
Moved variables setting for headers to a more appropriate section of the configuration.
Reducing macro name from "ONEAPI_ONEMKL" to "ONEMKL"
Description
This PR adds
ONEAPI_ONEMKL_prefix to all the variables defined inconfig.hpp.into be conformant with ES.33 C++ Core Guidelines.Fixes # (GitHub issue)
This change was requested in #554
Checklist
All Submissions
Do all unit tests pass locally? Attach a log.
portblas_log.txt
portfft_log.txt
nvidia_all_domains_log.txt
cpu_mkl_all_domains_log.txt
arc_mklgpu_all_domains_log.txt
Have you formatted the code using clang-format?