Closed amontoison closed 11 months ago
Excellent many thanks, I think initially we just need to set in config.h
(see #135):
HAVE_HWLOC -- used to detect NUMA regions
SPRAL_HAVE_METIS_H -- used to define METIS5 32bit/64bit index types
SIZEOF_IDX_T -- used to define METIS5 32bit/64bit index types
HAVE_NVCC -- used to build CUDA kernels
HAVE_SCHED_GETCPU -- used to check if sched_getcpu() can be used
although I would like to find a better way to detect METIS5 64bit index types (see #136).
Can we autodetect the METIS header with meson and check sizeof(idx_t)
?
Can we autodetect the METIS header with meson and check
sizeof(idx_t)
?
We are already autodetecting the METIS header for libHSL
:
libmetis_name = get_option('libmetis')
libmetis_path = get_option('libmetis_path')
libmetis_include = include_directories(get_option('libmetis_include'))
libmetis_version = get_option('libmetis_version')
# C and Fortran compilers
cc = meson.get_compiler('c')
fc = meson.get_compiler('fortran')
# Dependencies
libmetis = fc.find_library(libmetis_name, dirs : libmetis_path, required : false)
has_metish = cc.has_header('metis.h', include_directories : libmetis_include)
We can probably check the content of metis.h
to check if we have
#define IDXTYPEWIDTH 32
or
#define IDXTYPEWIDTH 64
In the same time, we can also check if we have METIS 4 or METIS 5 with METIS_VER_MAJOR
.
@jfowkes
We have a working Meson build system now.
HAVE_CONFIG_H
was missing in some C++ files, I modified them such we can compile SPRAL with Meson.
A few comments:
ralna/spral
, can you add it such that we use CI on FreeBSD and Apple-M1 platforms?ssids_only
(or another name) such that only ssids is compiled for Ipopt users, what do you think? It's also relevant for GALAHAD.config.h
, I found a way to add dynamically HAVE_HWLOC
and SPRAL_HAVE_METIS_H
.
SIZEOF_IDX_T
is generated by metis5_wrappers.F90 for us if SPRAL_HAVE_METIS_H
is found but we can add an option
for the users to modify it directly.
HAVE_NVCC
should be easy to add when we will work on GPU support.
For HAVE_SCHED_GETCPU
, I don't how we can check if sched_getcpu()
is available, any idea?ssmfe
requires CBLAS, we should update it to directly use Fortran routines. I pushed the file cblas.h
as hotfix.It's finally working! :tada: :tada: :tada:
Update: The logs for Windows: https://github.com/ralna/spral/suites/16773345740/artifacts/957859756
Test failure at ../tests/ssids/kernels/block_ldlt.cxx:221
ASSERT_EQ(rloc1, rloc2) failed
[(test_maxloc_torture<double, 128>(10000))[fail]]
@amontoison excellent many thanks! We are able to reproduce #144 from @bharswami so I will now file that as a bug. Would be nice to fix this before merging the meson build system.
@amontoison in answer to your comments:
hwloc
on Windows and Mac? We have hwloc working fine on Mac in the current CI tests...ralna/spral
ssids_only
option since 99% users only want SSIDSsched_get_cpu()
this is in glibc
(The C standard library) on Linux, so we can set HAVE_SCHED_GETCPU
to true on Linux platforms, not sure about Mac or Windows?
- Is there a reason you ditch
hwloc
on Windows and Mac? We have hwloc working fine on Mac in the current CI tests...
A header file hwloc.h is missing if I try to compile on Mac and Windows. I compiled without hwloc just to check for other errors.
- Happy to deal with GPU meson build once we have CPU meson build working.
Great!
- I have activated CirrusCI for
ralna/spral
Ok, I will try to compile SPRAL on FreeBSD and Apple M1.
- I also want an
ssids_only
option since 99% users only want SSIDS
Ok I will add it today.
- For
sched_get_cpu()
this is inglibc
(The C standard library) on Linux, so we can setHAVE_SCHED_GETCPU
to true on Linux platforms, not sure about Mac or Windows?
How HAVE_SCHED_GETCPU
is set by the autotools build system?
- I agree that SSMFE should not be relying on CBLAS but Fortran BLAS instead
Let's open an issue about that and update SSMFE in another PR.
A header file hwloc.h is missing if I try to compile on Mac and Windows. I compiled without hwloc just to check for other errors.
You just need to point Meson to the directory with hwloc.h
, on Mac you can use brew --prefix
.
How
HAVE_SCHED_GETCPU
is set by the autotools build system?
It includes <sched.h>
(from GLIBC or equivalent) and then tries to compile sched_getcpu()
:
AC_TRY_LINK([#include <sched.h>], [sched_getcpu();],
More details here: https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Obsolete-Macros.html
@jfowkes I have this error on FreeBSD.
Current memory used: 416 bytes
Maximum memory used: 416 bytes
***Memory allocation failed for OMETIS: cptr. Requested size: 10428522542804238376 bytes
Program received signal SIGSEGV: Segmentation fault - invalid memory reference.
Backtrace for this error:
#0 0x823820369 in ???
#1 0x82381f495 in ???
#2 0x82a913b5f in handle_signal
at /usr/src/lib/libthr/thread/thr_sig.c:303
#3 0x82a91311e in thr_sighandler
at /usr/src/lib/libthr/thread/thr_sig.c:246
#4 0x7ffffffff8a2 in ???
#5 0x822611240 in __spral_metis_wrapper_MOD_metis_order32
at ../src/metis5_wrapper.F90:170
I checked what we should not compile if we want to add an option ssids_only
and the difference is very small. We will not compile lsmr
and ssmfe
in that case. The issue is that we need to modify the header file spral.h
to not include lsmr.h
and ssmfe.h
.
I just need to add the HAVE_SCHED_GETCPU
in Meson to finalize the PR. (done :heavy_check_mark: )
@jfowkes I have this error on FreeBSD.
Current memory used: 416 bytes Maximum memory used: 416 bytes ***Memory allocation failed for OMETIS: cptr. Requested size: 10428522542804238376 bytes Program received signal SIGSEGV: Segmentation fault - invalid memory reference. Backtrace for this error: #0 0x823820369 in ??? #1 0x82381f495 in ??? #2 0x82a913b5f in handle_signal at /usr/src/lib/libthr/thread/thr_sig.c:303 #3 0x82a91311e in thr_sighandler at /usr/src/lib/libthr/thread/thr_sig.c:246 #4 0x7ffffffff8a2 in ??? #5 0x822611240 in __spral_metis_wrapper_MOD_metis_order32 at ../src/metis5_wrapper.F90:170
@amontoison I see errors like this when using 64bit integers but METIS5 is built with 32bit integer support and vice versa. Is the FreeBSD METIS 64bit integer? Or is Meson not correctly detecting the METIS headers and the SIZEOF_IDX_T
on FreeBSD?
I checked what we should not compile if we want to add an option
ssids_only
and the difference is very small. We will not compilelsmr
andssmfe
in that case. The issue is that we need to modify the header filespral.h
to not includelsmr.h
andssmfe.h
.
Okay let's skip an SSIDS only build for now, seems like it's not worth it atm.
I just need to add the
HAVE_SCHED_GETCPU
in Meson to finalize the PR. (done ✔️ )
Great stuff!
Oh, I throught that SIZEOF_IDX_T
was a variable of metis.h
but it's not the case.
I propose to replace the line https://github.com/ralna/spral/blob/master/src/metis5_wrapper.F90#L23 with
#if IDXTYPEWIDTH == 64
directly.
IDXTYPEWIDTH
is defined by the metis header.
Oh, I throught that
SIZEOF_IDX_T
was a variable ofmetis.h
but it's not the case.I propose to replace the line https://github.com/ralna/spral/blob/master/src/metis5_wrapper.F90#L23 with
#if IDXTYPEWIDTH == 64
directly.IDXTYPEWIDTH
is defined by the metis header.
Yeah sure, that seems sensible to me. Why wasn't this done in the first place? We should also add some tests where we test against 64bit integer METIS so we know it woks?
@amontoison also you will have to add CBLAS support for Meson, some of the SSMFE C examples require CBLAS functions for various matrix modification routines, eg:
for(int i=0; i<rci.nx; i++) {
if( rci.kx == rci.ky ) {
double s = cblas_dnrm2(n, &W[rci.kx][rci.jx+i][0], 1);
if( s > 0 )
cblas_dscal(n, 1/s, &W[rci.kx][rci.jx+i][0], 1);
} else {
double s = sqrt(fabs(cblas_ddot(
n, &W[rci.kx][rci.jx+i][0], 1, &W[rci.ky][rci.jy+i][0], 1)
));
if ( s > 0 ) {
cblas_dscal(n, 1/s, &W[rci.kx][rci.jx+i][0], 1);
cblas_dscal(n, 1/s, &W[rci.ky][rci.jy+i][0], 1);
} else {
for(int j=0; j<n; j++)
W[rci.ky][rci.jy+i][j] = 0.0;
}
}
}
@amontoison also you will have to add CBLAS support for Meson, some of the SSMFE C examples require CBLAS functions for various matrix modification routines, eg:
for(int i=0; i<rci.nx; i++) { if( rci.kx == rci.ky ) { double s = cblas_dnrm2(n, &W[rci.kx][rci.jx+i][0], 1); if( s > 0 ) cblas_dscal(n, 1/s, &W[rci.kx][rci.jx+i][0], 1); } else { double s = sqrt(fabs(cblas_ddot( n, &W[rci.kx][rci.jx+i][0], 1, &W[rci.ky][rci.jy+i][0], 1) )); if ( s > 0 ) { cblas_dscal(n, 1/s, &W[rci.kx][rci.jx+i][0], 1); cblas_dscal(n, 1/s, &W[rci.ky][rci.jy+i][0], 1); } else { for(int j=0; j<n; j++) W[rci.ky][rci.jy+i][j] = 0.0; } } }
Do you mean a specific library for cblas? We already support an option for blas and lapack. We just expect a library with both symbols right now like openblas, mkl or apple accelerate.
Do you mean a specific library for cblas? We already support an option for blas and lapack. We just expect a library with both symbols right now like openblas, mkl or apple accelerate.
Sorry I meant to ensure it detects the CBLAS header cblas.h
, this comes standard as part of all BLAS/LAPACK installations (it's just a C interface to the F77 BLAS routines).
Oh, I throught that
SIZEOF_IDX_T
was a variable ofmetis.h
but it's not the case. I propose to replace the line https://github.com/ralna/spral/blob/master/src/metis5_wrapper.F90#L23 with#if IDXTYPEWIDTH == 64
directly.IDXTYPEWIDTH
is defined by the metis header.Yeah sure, that seems sensible to me. Why wasn't this done in the first place?
I don't know, it seems obvious to check IDXTYPEWIDTH
directly.
We should also add some tests where we test against 64bit integer METIS so we know it works?
I added some tests with 64 bits integer METIS and it's not working.
I exactly have the same errors with FreeBSD, so I checked the metis.h
file on FreeBSD and it's indeed a version with 64 bits integer.
Do you mean a specific library for cblas? We already support an option for blas and lapack. We just expect a library with both symbols right now like openblas, mkl or apple accelerate.
Sorry I meant to ensure it detects the CBLAS header
cblas.h
, this comes standard as part of all BLAS/LAPACK installations (it's just a C interface to the F77 BLAS routines).
@jfowkes
I did a modification to ensure that the CLAS header cblas.h
is correctly detected before that we run the tests and the examples of ssfme
.
@amontoison when I build locally with 64bit METIS using autotools then I'm not seeing this issue, are you sure that SPRAL_HAVE_METIS_H
is set to True
? Otherwise it defaults to 32bit integers again...
@amontoison when I build locally with 64bit METIS using autotools then I'm not seeing this issue, are you sure that
SPRAL_HAVE_METIS_H
is set toTrue
? Otherwise it defaults to 32bit integers again...
Well spotted!
In metis5_adapter.F90, it's a #if SPRAL_HAVE_METIS_H
and not a #ifdef SPRAL_HAVE_METIS_H
.
@amontoison I think the issue is between the screen and the chair: we need to #include <metis.h>
in metis5_wrapper.F90
otherwise the wrapper cannot see IDXTYPEWIDTH
since it's defined in metis.h
right?
Also we can probably just ditch the SPRAL_HAVE_METIS_H
macro and rely on IDXTYPEWIDTH
existing or not right?
I checked with a grep -nr
the use of the variables SPRAL_HAVE_METIS_H
, HAVE_HWLOC
and HAVE_SCHED_GETCPU
.
We have both #idfef
and #if
in the files.
I set all of them to 1.
@amontoison I think the issue is between the screen and the chair: we need to
#include <metis.h>
inmetis5_wrapper.F90
otherwise the wrapper cannot seeIDXTYPEWIDTH
since it's defined inmetis.h
right?
Indeed, I pushed the fix.
If we ditch the macro SPRAL_HAVE_METIS_H
, the compilation will fail if the header metis.h
is not found.
Is it what you want?
Indeed, I pushed the fix. If we ditch the macro
SPRAL_HAVE_METIS_H
, the compilation will fail if the headermetis.h
is not found. Is it what you want?
Okay no let's not do that...
@jfowkes
We can't include the header file "metis.h" in Fortran code.
We can only use it in C or C++ files.
I added an option metis64
(boolean) in Meson to specify if METIS was compiled with 64 bits integers.
We can update it once the old build system is removed.
@amontoison ah of course :facepalm:
Many thanks for your work on this! I guess this is ready for review then?
Yes, It's ready now.
Great I'll add some documentation and merge it in!
close #129
I did the Meson build system tonight.
During the compilation I have an error because we need an header
config.h
that is generated from configure.ac by autoheader. We need to solve #135.The Meson build system will also help to solve https://github.com/conda-forge/spral-feedstock/pull/2.