-
Notifications
You must be signed in to change notification settings - Fork 548
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
New builder: SuperLU_MT #983
New builder: SuperLU_MT #983
Conversation
I am guessing double precision real is what you want. But perhaps more a question for @ChrisRackauckas. |
double precision real. |
cc @jd-lara |
Thanks guys. Seems that only Linux is building properly right now, it is okay to restrict compatibility to Linux or should we try to support anything else? I'm not sure what SuperLU_MT itself claims to support. |
If we don't get windows and mac support, that means we'll lose that for sundials too. |
I think what is required is to use gcc on all the platforms, if you use openmp. Mac otherwise uses clang. On windows seems like a link problem that is preventing it from finding BLAS. Should be easy to fix. |
Ah, now that I look at the errors, they are not so scary... I'll look into getting them fixed 🙂 |
On MacOS with
|
Maybe I'm missing something, but the errors seem due to the fact that it's not linking to openblas nor libgomp. Also, the names of the symbols will be wrong, they miss the trailing 64 |
Oops, I should definitely have used |
I can also get rid of the error about |
You have to link to openblas, but also rename all the symbols. This is the kind of dance you need to do. I'm surprised you didn't have to for Linux |
Do make sure SuperLU supports a 64-bit BLAS. Otherwise, link against OpenBLAS32. |
Looks like the symbol renaming is indeed working! Taking a little screen break but I should have an update tonight. |
You do need to make sure it can pass 64-bit integers to blas in the build system. |
The only info I can find about 64-bit BLAS compatibility is the following:
I've defined that on 64-bit systems that are not aarch64. Is that sufficient or is there more stuff I should be checking? |
Good to go? |
Just looking for @ViralBShah's approval on whether I've done the appropriate check for 64-bit BLAS. |
It looks like they don't support a 64-bit BLAS. I only see 32-bit ints everywhere. So best to go with OPENBLAS32_jll. The The other thing to do is to build 4 versions of the library for each different precision and just give each one a different name. That way we put that worry behind us, and all precisions are available. That is how we do it in the other sparse solvers like MUMPS too. |
So maybe it's better to reorganise the scripts so that there is a single common file that all the different versions include? |
It would seem that |
Ah never mind, the |
Ok, 32-bit BLAS it is. I can go the route of 4 separate products too if we want. Should I go for it? |
I think it would be great if you could. Simpler to do it when you are in the midst of the build system. |
I am pretty sure we will need to build Sundials with only 32-bit OpenBlas to make all of the stack compatible. |
Ok, on it! |
I am curious to see how it works out. Currently sundials uses openblas with mangled names for 64-bit, and 32-bit names should thus not clash. It is possible that it just works out. I would at least love to try it out and see what happens. It may also not be too hard to make superlu call 64-bit BLAS. |
From the sundials manual: " |
Just re-clarifying - that is only applicable to the index type in the sparse matrix data structure and not the BLAS (Both in the case of sundials and superlu) |
Yes, this applies to the size of the matrix. Basically, in theory we can use Sundials with arrays size n=2^64-1 but I have had problems when mixing 32-bit and 64-bit indexed packages even for small problems in the past. |
@jd-lara When we were building sundials, we did not have openblas32_jll. Now that we have it, we can move to 32-bit blas in sundials across the board. I agree that should be sufficient. |
Ok, this will also be relevant for #845 since this PR and PETSc are both relevant to expand the capabilities of Sundials. |
S/SuperLU_MT/build_tarballs.jl
Outdated
echo "NOOPTS += -fPIC -fopenmp \$(BLASLIB)" >> make.inc | ||
|
||
# On 64-bit systems, use 64-bit integers for indexing. | ||
if [[ "$nbits" == 64 ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are building this for Sundials and modify Sundials to work with only 32-Bits
"
Moreover, since the superlumt library may be installed to support either 32-bit or 64-bit integers, it is assumed that the superlumt library is installed using the same integer precision as the sundials sunindextype option
"
cc. @ViralBShah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it makes sense to make these binaries 32-bit only to avoid the 32/64-bit mess. @ChrisRackauckas fyi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we're doing 32-bit everywhere, and so the -D_LONGINT
should go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's do that.
I think this should be good to go now? Seems like the next bit of experimentation is to do with Sundials. |
Yeah we need to make Sundials use OpenBlas32 and force the indexes to 32bit. I can try to do that over the weekend. |
I had a quick go at it last night, all I did was change the OpenBLAS_jll dependency, change the lapack library path to never have the 64_ suffix, and didn't apply the Fortran patch that added the suffixes. It built fine, but I tried to run Sundials tests with it and it errored out complaining about |
That probably just needs to expand the libgfortran versions or something, and add a dependency on CompilerSupportLibraries. |
Looks like you were right, it loads properly after compiling the different libgfortran versions. I do get one test failure though. |
32-bit sundials discussion can continue in #995 |
@giordano I think this is good to merge now. |
We may have to revisit the openmp choice later. |
This is primarily for Sundials. I've left all the platforms in here, although I am 99% sure that plenty will fail -- I mostly want to use the CI to find out 😅
Question for @ViralBShah and/or @ChrisRackauckas: SuperLU_MT supports four precisions:
It doesn't seem like they can coexist; trying to compile them together into one shared library fails because each precision defines functions with the same name.
Is there a preference for which precision is used? I just used single-real while I was getting it working but they all compile successfully on their own. I'm not really sure if using complex means that it no longer supports real numbers or something...