DrTimothyAldenDavis / SuiteSparse

The official SuiteSparse library: a suite of sparse matrix algorithms authored or co-authored by Tim Davis, Texas A&M University.
https://people.engr.tamu.edu/davis/suitesparse.html
Other
1.15k stars 259 forks source link

BLAS_SUFFIX ignored when passing your own BLAS_LIBRARIES variable #668

Closed imciner2 closed 8 months ago

imciner2 commented 8 months ago

Describe the bug When building using CMake variables to define your own BLAS libraries and their linkage (e.g. passing -DBLAS_LIBRARIES=<blah> on the command line), the SuiteSparseBLAS.cmake file does not configure any of the compile definitions, such as the BLAS_SUFFIX, or call the 64-bit/32-bit BLAS CMake files.

To Reproduce In the Julia build, we pass in -DBLAS_LIBRARIES with the libblastrampoline file, and also set -DBLAS64_SUFFIX="_64" and -DSUITESPARSE_USE_64BIT_BLAS=YES on the command line. However, the actual BLAS_SUFFIX compile definition is not being set when compiling, so the libraries are just linking to dgemm_ instead of dgemm_64_ like we want.

The CMake trace (e.g. running CMake with --trace --trace-expand) for a sample build showing how it is processing the BLAS/LAPACK files is:

[18:41:17] /workspace/srcdir/SuiteSparse/SPQR/CMakeLists.txt(61):  include(SuiteSparseBLAS )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(18):  cmake_minimum_required(VERSION 3.22 )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(21):  if(DEFINED ENV{BLA_VENDOR} )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(24):  set(BLA_VENDOR ANY CACHE STRING if ANY (default): searches for any BLAS. Otherwise: search for a specific BLAS )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(28):  option(SUITESPARSE_USE_64BIT_BLAS OFF (default): use only 32-bit BLAS.  ON: look for 32 or 64-bit BLAS OFF )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(32):  option(BLA_STATIC OFF (default): dynamic linking of BLAS.  ON: static linking of BLAS OFF )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(35):  if(DEFINED BLAS_LIBRARIES OR DEFINED BLAS_INCLUDE_DIRS )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(38):  if(SUITESPARSE_USE_64BIT_BLAS )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(39):  set(SuiteSparse_BLAS_integer int64_t )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake(43):  return()
[18:41:17] /workspace/srcdir/SuiteSparse/SPQR/CMakeLists.txt(62):  include(SuiteSparseLAPACK )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseLAPACK.cmake(21):  cmake_minimum_required(VERSION 3.22 )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseLAPACK.cmake(23):  if(DEFINED LAPACK_LIBRARIES OR DEFINED LAPACK_INCLUDE_DIRS )
[18:41:17] /workspace/srcdir/SuiteSparse/SuiteSparse_config/cmake_modules/SuiteSparseLAPACK.cmake(26):  return()

I would guess that there should be calls to SuiteSparseBLAS32/64 in that if statement to configure the suffixes and other options for the BLAS library.

Desktop (please complete the following information):

mmuetzel commented 8 months ago

Thanks for reporting and tracking this down.

Afaict, #671 should fix this.

DrTimothyAldenDavis commented 8 months ago

Would it be helpful to post a SuiteSparse 7.5.0.betaX with this change?

imciner2 commented 8 months ago

Since the patch was so small, I have actually backported it as a patch to our 7.4.0 build and tested it. After the build, I now see libspqr using the functions

                 U dlarf_64_
                 U dlarfb_64_
                 U dlarfg_64_
                 U dlarft_64_
                 U dnrm2_64_
                 U dznrm2_64_

so it looks like the suffix is being added properly now.

DrTimothyAldenDavis commented 8 months ago

Great!

I've posted a 7.5.0.beta3 with this fix: https://github.com/DrTimothyAldenDavis/SuiteSparse/releases/tag/v7.5.0.beta3 , if you'd like to give that a try.