spack / spack

A flexible package manager that supports multiple versions, configurations, platforms, and compilers.
https://spack.io
Other
4.07k stars 2.17k forks source link

intel-oneapi-mkl: select interface based on compiler #43673

Closed RMeli closed 1 month ago

rscohn2 commented 1 month ago

I am checking with someone in the MKL team

RMeli commented 4 weeks ago

I created this PR because of wrong results with a Fortran interface I'm writing for a C++ library. However, this PR seems to have created issues on the upstream C++ library. The link-line advisor and the MKLConfig.cmake both suggest/use libmkl_intel_* for C/C++ applications and only use libmkl_gf_* for Fortran applications with gfortran (GNU_Fortran_COMPILER in MKLConfig.cmake). Our observation is that for C/C++ applications it doesn't matter if we use libmkl_intel_* or libmkl_gf_*, but they can't co-exist. Is this correct? If yes, what is the recommended way to deal with situations where a C++ library uses MKLConfig.cmake (which links against libmkl_intel_*) and a downstream Fortran project with gfortran links against libmkl_gf_* (also using MKLConfig.cmake)? Should this PR have been a user-controlled variant instead?

rscohn2 commented 4 weeks ago

When you install intel-oneapi-mkl%oneapi or intel-oneapi-mkl%gcc the only difference is the selection of the fortran compatibility and the openmp implementation. Using the toolchain instead of an explicit variant seems to be a common spack practice and would usually do the right thing. We could change it to a variant, or keep the toolchain logic, but add a variant to override. I don't have a strong opinion on this. What do you prefer? Note that the current logic does not support intel fortran with gnu omp, or gfrotran and intel omp.

RMeli commented 3 weeks ago

I don't have a strong preference, I was just wondering if a variant would be better for the end user.

I think the fact that MKLConfig.cmake always uses libmkl_intel_* for C/C++ projects might be somewhat problematic, in combination with this PR. For example, after this PR, we were having issues with blaspp linking to libmkl_gf_* while the downstream package dla-future was linking with libmkl_intel_*, leading to segmentation faults. Is it correct to conclude that they both work for C/C++ projects, but can't be mixed?

I think the issue surfaced because after this PR blaspp now links with libmkl_gf_* because it is using https://github.com/spack/spack/blob/e9149cfc3cfc94bd66aa0e0901ce562cf8b75713/var/spack/repos/builtin/packages/blaspp/package.py#L102 while dla-future is relying on MKLConfig.cmake https://github.com/spack/spack/blob/e9149cfc3cfc94bd66aa0e0901ce562cf8b75713/var/spack/repos/builtin/packages/dla-future/package.py#L170-L175 which seems to always provide libmkl_intel_* unless GNU_Fortran_COMPILER is defined. Before this PR we didn't see this issue, because blaspp was also linking to libmkl_intel_*.

In MKLConfig.cmake we can only specify MKL_INTERFACE=lp64 or MKL_INTERFACE=ilp64 and the GNU or Intel interface is also selected based only on the Fortran the compiler.

Should we stop relying on MKLConfig.cmake in our Spack package? What is the best way forward to avoid this kind of issues?

rscohn2 commented 3 weeks ago

I looked at MKLConfig.cmake. My understanding is that it detects the compiler (gnu or intel) and then that determines which omp/fortran to be compatible with. The change in this PR makes the spack package behave the same way. That seems like the right default behavior.

Would it work if dla-future and intel-oneapi-mkl had the same compiler? These are ok:

This is true for all packages that use fortran.

rscohn2 commented 3 weeks ago

Is it correct to conclude that they both work for C/C++ projects, but can't be mixed?

Correct.

RMeli commented 3 weeks ago

The change in this PR makes the spack package behave the same way.

I'm not sure this is the case. This PR selects the interface based on the compiler, independently of the language. While my understanding is that MKLConfig.cmake only uses the Fortran compiler (GNU_Fortran_COMPILER variable) to determine the interface. Therefore, for a pure C/C++ project like dla-future, using MKLConfig.cmake always results in linking against libmkl_intel_* and therefore might result in the issues we observed (we are not mixing compilers).

rscohn2 commented 3 weeks ago

I see this in MKLConfig.cmake:

  if(CMAKE_Fortran_COMPILER_ID STREQUAL "GNU")
    set(GNU_Fortran_COMPILER ON)
  endif()

When it builds dla-future, Spack will define either F77=gfortran or F77=ifx depending on whether it is dla-future%gcc or dla-future%oneapi and that will determine whether cmake will use libmkl_gf or libmkl_intel

Other packages that depend on intel-oneapi-mkl and use spec['blas'].libs to get the libraries will need either intel-oneapi-mkl%gcc or intel-oneapi-mkl%intel so it matches dla-future

I could be misunderstanding what actually happens. Having both MKL and CMake picking the libraries, and relying on indirect ways to say what you want seem bad. I don't know what would work better. If you think defaulting to libmkl_intel unless a GF variant was provided to intel-oneapi-mkl is better, I can change it.

RMeli commented 3 weeks ago

Thank you for your insight. I did a quick and small test to check what happens with CMAKE_Fortran_COMPILER_ID. I think the issue is that CMAKE_Fortran_COMPILER_ID is not defined/empty if the Fortran language is not enabled in CMake.

cmake_minimum_required(VERSION 3.22)
project(test VERSION 1.0 LANGUAGES CXX C)
message("Compiler ID: ${CMAKE_Fortran_COMPILER_ID}")

results in

Compiler ID:

while

cmake_minimum_required(VERSION 3.22)
project(test VERSION 1.0 LANGUAGES CXX C Fortran)
message("Compiler ID: ${CMAKE_Fortran_COMPILER_ID}")

results in

Compiler ID: GNU

dla-future only has C and C++ enabled, so I think that's why it is still linking with libmkl_intel_* regardless of what Spack says.

Therefore, this PR can lead to mixing libmkl_intel_* and libmkl_gf_* for C/C++ packages relying on MKLConfig.cmake (with dependencies using spec['blas'].libs instead). But without this PR libmkl_intel_* was always provided, which is also problematic.

I'm not sure what the best way to ensure consistency is. Maybe having MKLConfig.cmake also use CMAKE_C_COMPILER_ID/CMAKE_CXX_COMPILER_ID to determine the interface would be an improvement? But probably there are other consequences I'm not considering.

rscohn2 commented 3 weeks ago

Your experiment makes the problem clearer. cmake projects that do not use fortran will use libmkl_intel. CMake projects that use gfortran will use libmkl_gf. It will be difficult to change the cmake behavior. I want to change the spack package. At the beginning of the thread, you proposed using a variant instead of toolchain to trigger use of libmkl_gf and that seems like the best choice.