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

Add support for optional MKL and support for aarch64 #115

Merged
merged 9 commits into from
Sep 22, 2020

Conversation

mathomp4
Copy link
Member

@mathomp4 mathomp4 commented Jul 28, 2020

This PR adds support for optional MKL as well as preliminary support for aarch64 (Graviton2) processors with GNU

Should be taken in concert with:

GEOS-ESM/NCEP_Shared#8
GEOS-ESM/GEOSgcm_GridComp#321

ETA: Turns out thanks to CMake knowledge of @tclune this is no longer true! Since each component specifies what they need, they are now independent!

mathomp4 added 2 commits June 10, 2020 16:47
This set of PRs is to allow GEOS to build without MKL. Note that it currently does this by making RRTMGP unusable if MKL is not found. This is not ideal, but it's a first attempt.
@mathomp4 mathomp4 added the enhancement New feature or request label Jul 28, 2020
@mathomp4 mathomp4 self-assigned this Jul 28, 2020
@mathomp4 mathomp4 changed the title Add support for optional MKL Add support for optional MKL and support for aarch64 Sep 1, 2020
@mathomp4
Copy link
Member Author

mathomp4 commented Sep 2, 2020

Pinging @lizziel and @LiamBindle.

I wanted to bring this to your attention. I've been building/running GEOS on an Arm64 machine at AWS and as such, I've had to make some branches to avoid an MKL dependency in GEOS (in RRTMGP). Now, you probably don't care much about that, but in order to get other bits of GEOS to build, I found that we had a semi-undocumented LAPACK dependency. Thus, this PR will do more than find_package(MKL) but also find_package(LAPACK) and find_package(BLAS). I want to make sure you know this and it doesn't break anything on your end. It's pretty passive, but...

Also, it adds a bit of change the GNU.cmake where I can't exactly use -march=westmere on an Arm processor. :)

@mathomp4
Copy link
Member Author

mathomp4 commented Sep 2, 2020

Also, I'm pinging you because this set of PRs might go in soon.

@LiamBindle
Copy link

LiamBindle commented Sep 2, 2020

@mathomp4 Thanks for the heads up! Just to make sure I understand this correctly, this doesn't introduce MKL/LAPACK/BLAS as a new dependency in MAPL right? It's just the CMakeLists will include find_package() calls to find them since GEOS needs it?

@mathomp4
Copy link
Member Author

mathomp4 commented Sep 2, 2020

@LiamBindle MAPL doesn't care at all. But, technically, MKL has always been a required dependency of using ESMA_cmake (or include(esma)). Now it wouldn't be (though you'd lose functionality of RRTMGP), and makes BLAS and LAPACK as required.

@mathomp4
Copy link
Member Author

mathomp4 commented Sep 2, 2020

@LiamBindle If need be, I could code in an option that would allow for skipping this altogether? Something like:

option(USE_BLASLAPACK "disable BLAS and LAPACK detection" ON)

if you really don't want any BLAS or LAPACK requirement? Or do you not even do include(esma)?

@LiamBindle
Copy link

@mathomp4 Sounds good, this shouldn't cause a problem on our end. We're using #57 which does something similar to disable find_package() calls that we don't actually need. In the near future I'm planning to revise our integration of the GEOS-ESM build system into GCHPctm in a way that's less intrusive than #57, with the compiler option ordering in mind, and because it seems like there have been lots of good updates in the last few months!

Thanks again for the heads up!

@tclune
Copy link
Collaborator

tclune commented Sep 2, 2020

I wonder if the right thing to do in this regard is the following:

  • No find_package() in the top esma.cmake except maybe MPI?
  • Layers that have dependencies issue find_package() as appropriate.

It does not hurt to repeat these and it will make the dependencies more correct under random configurations. We probably cannot fix this overnight - or would not want to take such a risk. But over time we should migrate these.

@LiamBindle
Copy link

LiamBindle commented Sep 2, 2020

@tclune I love that idea. Working towards that would help on our end since it would decouple MAPL's build from GEOS.

@mathomp4
Copy link
Member Author

mathomp4 commented Sep 2, 2020

I wonder if the right thing to do in this regard is the following:

  • No find_package() in the top esma.cmake except maybe MPI?
  • Layers that have dependencies issue find_package() as appropriate.

It does not hurt to repeat these and it will make the dependencies more correct under random configurations. We probably cannot fix this overnight - or would not want to take such a risk. But over time we should migrate these.

Yes, indeed. And doing that would allow us to better do PUBLIC/PRIVATE as well. For example, the NCEP_Shared PR is a PUBLIC which...ugh. But, if we make everything PRIVATE that might be optional (like MKL/BLAS/LAPACK) then we could help isolate these things and say, okay, NCEP_sp should find_package(LAPACK REQUIRED) and maybe RRTMG would find_package(MKL).

(Of course, things like MPI are just "required" at all levels and goes up to the top.)

@mathomp4 mathomp4 marked this pull request as ready for review September 4, 2020 12:42
@mathomp4 mathomp4 requested a review from a team as a code owner September 4, 2020 12:42
@mathomp4 mathomp4 marked this pull request as draft September 4, 2020 12:49
@mathomp4 mathomp4 added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs labels Sep 4, 2020
tclune
tclune previously approved these changes Sep 4, 2020
@mathomp4 mathomp4 marked this pull request as ready for review September 4, 2020 12:58
@mathomp4
Copy link
Member Author

mathomp4 commented Sep 9, 2020

Another edit is incoming from/inspired by Dan Kokron. Technically, esma.cmake shouldn't have REQUIRED on find_package(BLAS) and find_package(LAPACK). Why? Well, take GEOSfvdycore. It doesn't need MKL, BLAS, or LAPACK. But this branch makes it required.

Now that NCEP_sp is the one requiring LAPACK, that should cause the fault in CMake, not the esma.cmake step. Testing now.

Instead, the CMakeLists.txt in the package that requires BLAS or LAPACK
should REQUIRED it.
@mathomp4 mathomp4 added 0 diff structural Structural changes to repository that are zero-diff and removed Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs labels Sep 22, 2020
@mathomp4 mathomp4 requested a review from tclune September 22, 2020 12:39
@tclune tclune merged commit 5129855 into develop Sep 22, 2020
@tclune tclune deleted the feature/mathomp4/mkl-is-optional branch September 22, 2020 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff structural Structural changes to repository that are zero-diff 0 diff The changes in this pull request have verified to be zero-diff with the target branch. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants