Skip to content

Add support for BLAS/LAPACK ILP64 libraries#16

Merged
awvwgk merged 14 commits intogrimme-lab:mainfrom
dmejiar:updates
Jun 15, 2022
Merged

Add support for BLAS/LAPACK ILP64 libraries#16
awvwgk merged 14 commits intogrimme-lab:mainfrom
dmejiar:updates

Conversation

@dmejiar
Copy link
Copy Markdown
Contributor

@dmejiar dmejiar commented Apr 11, 2022

No description provided.

@awvwgk awvwgk self-requested a review April 11, 2022 16:19
@awvwgk
Copy link
Copy Markdown
Member

awvwgk commented Apr 11, 2022

Do we need the code duplication of blas.f90 and blas64.f90 here? It would be preferable if we can just have a single file rather than two to avoid divergence.

@dmejiar
Copy link
Copy Markdown
Contributor Author

dmejiar commented Apr 11, 2022

It is not necessary. Let me remove blas64.f90 and work with macros

@awvwgk
Copy link
Copy Markdown
Member

awvwgk commented Apr 11, 2022

You could define a kind parameter ik and use it instead of i8

For the macro I would go with something along the lines of

#ifndef IK
#define IK i4
#endif

module mchrg_blas
  use mctc_env, only : sp, dp, ik => IK
  implicit none

  ! ...

end module mchrg_blas

@dmejiar
Copy link
Copy Markdown
Contributor Author

dmejiar commented Apr 11, 2022

Let me know if this works for you. If this is ok with you, I would also prepare PR's for dftd4, s-dftd3, and tblite with the same structure to enable ILP64

CMakeLists.txt Outdated
project(
"multicharge"
LANGUAGES "Fortran"
LANGUAGES "Fortran" "C"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer to only depend on a Fortran compiler here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have removed this in my latest commit, but the automatic MKL detection with cmake will not longer work. See bottom of https://cmake.org/cmake/help/latest/module/FindLAPACK.html

@dmejiar
Copy link
Copy Markdown
Contributor Author

dmejiar commented Apr 15, 2022

Actually, I was thinking the macros are not needed. If someone (like me) wants to use the ILP64 interface, then one can handle that with the -Dilp64=true and giving the right fortran flags, i.e. -Dfortran_flags="-fdefault-integer-8.

What's your preference @awvwgk ? I can close this PR and open a new one with a clean commit history

@awvwgk
Copy link
Copy Markdown
Member

awvwgk commented Apr 15, 2022

Indeed, promoting default integers to 8 bytes (-i8 for Intel and -fdefault-integer-8 for GCC) and using the custom library option should make this possible without changes in the build files already.

However, I'm still open to officially support ilp64 with the tblite stack as well.

dmejiar added a commit to dmejiar/dftd4 that referenced this pull request Jun 14, 2022
@dmejiar dmejiar mentioned this pull request Jun 14, 2022
dmejiar added a commit to dmejiar/tblite that referenced this pull request Jun 14, 2022
@awvwgk
Copy link
Copy Markdown
Member

awvwgk commented Jun 15, 2022

If the C compiler is needed for MKL detection, we could add it optionally in Findcustom-blas.cmake, if the correct BLA_VENDOR is set.

@dmejiar
Copy link
Copy Markdown
Contributor Author

dmejiar commented Jun 15, 2022

The following works for either LP64 or ILP64 if cmake >= 3.22 and $MKLROOT is set. For older versions of cmake the user should provide the correct BLA_VENDOR

diff --git a/config/cmake/Findcustom-blas.cmake b/config/cmake/Findcustom-blas.cmake
index 3dfe5c1..7f71e62 100644
--- a/config/cmake/Findcustom-blas.cmake
+++ b/config/cmake/Findcustom-blas.cmake
@@ -13,6 +13,14 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+if ((BLA_VENDOR MATCHES ^Intel) OR (DEFINED ENV{MKLROOT}))
+  enable_language("C")
+endif()
+
+if(WITH_ILP64)
+  set(BLA_SIZEOF_INTEGER 8)
+endif()
+
 if(NOT BLAS_FOUND)
   find_package("BLAS")
   if(NOT TARGET "BLAS::BLAS")
diff --git a/config/cmake/Findcustom-lapack.cmake b/config/cmake/Findcustom-lapack.cmake
index c78569d..8cd9a6a 100644
--- a/config/cmake/Findcustom-lapack.cmake
+++ b/config/cmake/Findcustom-lapack.cmake
@@ -13,6 +13,14 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+if ((BLA_VENDOR MATCHES ^Intel) OR (DEFINED ENV{MKLROOT}))
+  enable_language("C")
+endif()
+
+if(WITH_ILP64)
+  set(BLA_SIZEOF_INTEGER 8)
+endif()
+
 if(NOT LAPACK_FOUND)
   find_package("LAPACK")

@awvwgk
Copy link
Copy Markdown
Member

awvwgk commented Jun 15, 2022

This seems like a good solution to me.

Copy link
Copy Markdown
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Thanks for this patch. Looks good to me now.

@awvwgk awvwgk merged commit 4281891 into grimme-lab:main Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants