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

Update compilation flags for MKL/LAPACK #114

Closed
Andres-MG opened this issue Dec 14, 2022 · 5 comments · May be fixed by #119
Closed

Update compilation flags for MKL/LAPACK #114

Andres-MG opened this issue Dec 14, 2022 · 5 comments · May be fixed by #119
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@Andres-MG
Copy link
Collaborator

The current Makefile does not work with these flags:

  • COMPILER=gfortran
  • WITH_MKL=y

There is also a compilation warning with:

  • COMPILER=ifort
  • WITH_MKL=y

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.

@Andres-MG Andres-MG added bug Something isn't working good first issue Good for newcomers labels Dec 14, 2022
@Andres-MG Andres-MG self-assigned this Dec 14, 2022
@loganoz
Copy link
Owner

loganoz commented Dec 14, 2022

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

@Andres-MG
Copy link
Collaborator Author

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
Copy link
Owner

loganoz commented Dec 15, 2022

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
Copy link
Collaborator Author

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:

  • HAS_MKL enables MKL and LAPACK regions of the code.
  • HAS_LAPACK enables LAPACK regions of the code.

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

@loganoz
Copy link
Owner

loganoz commented Dec 21, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants