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

intel-oneapi-mkl: change logic for gfortran compatibility #44242

Merged
merged 4 commits into from
May 29, 2024

Conversation

rscohn2
Copy link
Member

@rscohn2 rscohn2 commented May 17, 2024

Use a variant to request gfortran compatibility instead of toolchain.
See discussion in #43673

@RMeli: Can you test your example. I testing: spack install dla-future ^intel-oneapi-mkl

@alalazo alalazo self-assigned this May 17, 2024
@alecbcs alecbcs changed the title [intel-oneapi-mkl]: change logic for gfortran compatibility intel-oneapi-mkl: change logic for gfortran compatibility May 17, 2024
@RMeli
Copy link
Member

RMeli commented May 20, 2024

@rscohn2 thank you for opening this PR. Sorry for the late reply, but I'm out of office until Wednesday. I'll have a look when I'm back.

@RMeli
Copy link
Member

RMeli commented May 27, 2024

@rscohn2 I see that there are still un-resolved conversations with @alalazo. Should I test this PR now or when they are resolved? (I would prefer the latter.)

From our discussion in #43673 I think a variant might indeed be better for users, since the default behavior will stay the same and not break pure C/C++ projects.

However, I don't think there is a way to make C/C++ projects relying in MKLConfig.cmake to work with downstream Fortran projects. Can we add a warning about this when using the +fortran variant?

@rscohn2
Copy link
Member Author

rscohn2 commented May 28, 2024

@alalazo: This is a binary package that does not require compilation. MKL put the C & Fortran support in the same library so we have to know whether to provide libmkl_gf or libmkl_intel. Some packages query spack for the libraries and some packages use cmake to know the mkl library names. If intel-oneapi-mkl returns libmkl_gf and another package uses cmake and links with libmkl_intel, there will be undefined behavior. cmake uses libmkl_intel unless a packages enables fortran and configures gfortran. Until recently, we did not support gfortran in the spack package, so it always supported C (gcc/intel/oneapi) and ifx/ifort.

I don't think spack has enough information to decide which library is needed so I would like to have an explicit variant like gfortran so the user can decide and be sure they are getting what they want.

@alalazo alalazo dismissed their stale review May 28, 2024 14:45

Modeling of the runtime can be improved, but it will take me a bit to get to it

@alalazo
Copy link
Member

alalazo commented May 28, 2024

@RMeli @rscohn2 Feel free to proceed as soon as this looks good to you. When we'll be at modeling runtimes, I'll be sure to revisit this and ping you both.

@rscohn2 rscohn2 marked this pull request as ready for review May 28, 2024 17:41
@rscohn2
Copy link
Member Author

rscohn2 commented May 28, 2024

@RMeli: If it looks ok, can you approve?

RMeli
RMeli previously approved these changes May 28, 2024
Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rscohn2, LGTM.

But as I said before, it would be good to add some sort of user-facing warning when using intel-oneapi-mkl with gcc, pointing to the possibility to enable this variant.

@rscohn2
Copy link
Member Author

rscohn2 commented May 28, 2024

But as I said before, I would be good to add some sort of user-facing warning when using intel-oneapi-mkl with gcc, pointing to the possibility to enable this variant.

Thanks for the reminder.

Co-authored-by: Rocco Meli <r.meli@bluemail.ch>
@rscohn2
Copy link
Member Author

rscohn2 commented May 29, 2024

Using fortran-rt check from @alalazo, I added a warning if +gfortran is not used.

@RMeli
Copy link
Member

RMeli commented May 29, 2024

Thank you, I'll test it right now.

RMeli
RMeli previously approved these changes May 29, 2024
Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could confirm that with ~gfortran the old behavior appears, and +gfortran fixes my issue.

I did not see any warning/message though.

@rscohn2
Copy link
Member Author

rscohn2 commented May 29, 2024

I did not see any warning/message though.

As a test, I made blaspp depend on fortran:

    depends_on("fortran", type="build")    

and then did:

spack install blaspp intel-oneapi-mkl ~gfortran

It does not emit a warning:

        if self.spec.satisfies("^[virtuals=fortran-rt] gcc-runtime"):                                                                      
                tty.warn("Use intel-oneapi-mkl +gfortran for GNU Fortran support")     

If I emit the warning for self.spec.satisfies("%gcc") then it will be emitting the warning when fortran is not involved and it will confuse people. I don't see a good way to warn.

@RMeli
Copy link
Member

RMeli commented May 29, 2024

I don't see a good way to warn.

Thanks for double checking. Let's just remove the warning then?

@rscohn2
Copy link
Member Author

rscohn2 commented May 29, 2024

I removed the warning, can you approve 1 more time?

Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@RMeli RMeli disabled auto-merge May 29, 2024 18:47
@RMeli RMeli enabled auto-merge (squash) May 29, 2024 18:47
@RMeli
Copy link
Member

RMeli commented May 29, 2024

Sorry, I disabled auto-merge by mistake. I already re-enabled it.

@RMeli RMeli merged commit 1184de8 into spack:develop May 29, 2024
14 checks passed
@rscohn2 rscohn2 deleted the dev/mkl branch May 29, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants