loganoz / horses3d

HORSES3D: A high-order discontinuous Galerkin solver for flow simulations and multi-physics applications
https://loganoz.github.io/horses3d/
MIT License
112 stars 24 forks source link

Update compilation flags for MKL/LAPACK #114

Closed Andres-MG closed 9 months ago

Andres-MG commented 1 year ago

The current Makefile does not work with these flags:

There is also a compilation warning with:

I propose changing the flag to something like WITH_LAPACK, using MKL for ifort and LAPACK for gfortran. The warning is easily fixed by changing the compilation flag from -mkl to -qmkl.

loganoz commented 1 year ago

I think we will need to adjust the automatic test cases with the update. What's the difference between -mkl and -qmkl?

Andres-MG commented 1 year ago

It seems that intel is deprecating -mkl in favor of -qmkl. We can leave the name of the flag as it is now, I just think it is a bit misleading since we are not using MKL in gfortran. The real issue is that the flags are wrong when using gfortran:

ifeq '$(WITH_MKL)''y'
    ifeq '$(FC)''gfortran'
        MACROS += -DHAS_MKL -DHAS_LAPACK
        LIBS += $(LIB_BLAS) $(LIB_LAPACK)
    else #ifort
        MACROS += -DHAS_MKL -DHAS_LAPACK
        FFLAGS += -mkl
    endif
endif

I think that -DHAS_MKL should not be set in that case, and LIB_BLAS and LIB_LAPACK seem to be empty.

loganoz commented 1 year ago

Feel free to modify the makefile to something that works. About the exact name of the flag in makefile, you can also change the WITH_MKL to WITH_LAPACK. If you do it, please, update the automated workflow, (which uses WITH_MKL now) and add this info to the manual (I think it is missing now). Thanks.

Andres-MG commented 1 year ago

Checking the tests I have realized that we never test the combinations gfortran/mkl and ifort/no mkl:

    strategy:
      fail-fast: true
      matrix:
        compiler: ['gfortran', 'ifort']
        mode: ['RELEASE']
        comm: ['SEQUENTIAL']
        enable_threads: ['YES']
        mkl: ['y','n']
        exclude:
        - compiler: gfortran
          mode: RELEASE
          comm: SEQUENTIAL
          enable_threads: YES
          mkl: 'y'
        - compiler: ifort
          mode: RELEASE
          comm: SEQUENTIAL
          enable_threads: YES
          mkl: 'n'

I am confused about this issue, I don't know what is the reason to combine MKL and LAPACK in the same flag. LAPACK is used only in DenseMatUtilities for basic linear algebra operations, while we require MKL for DFTs, sparse arrays or the Pardiso solver. However, I am not familiar with these parts of the code and it may be true that, in HORSES3D, LAPACK is only required when MKL is linked. If that is the case, I guess we can leave it as it is now.

If we split the HAS_MKL flag I think we should try something like this:

We could then test only gfortran/lapack and ifort/mkl to avoid increasing the testing time.

loganoz commented 1 year ago

I created the test, and I don't remember exactly why I did that. However, I have the following suspicion: a) gfortran is only tested without mkl because I couldn't make it work with mkl. b) ifort is not tested without mkl because that extra test does not add any information.

About the MKL vs LAPACK thing, I didn't know there where separated in the code. I added the MKL flag just to be able to use LAPACK routines.