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
msolve: more platforms, enable OpenMP (WIP) #3778
Conversation
Unfortunately FLINT_jll lacks an M1 binary so I first have to take care of that... UPDATE: see PR #3779 |
Huh, |
2ab5941
to
1bdf4b5
Compare
6e76328
to
18bfdce
Compare
Fixed the "gmp.h" errors by setting |
# define _SORT_R_BSD | ||
-#elif (defined _GNU_SOURCE || defined __gnu_hurd__ || defined __GNU__ || \ | ||
- defined __linux__ || defined __MINGW32__ || defined __GLIBC__) | ||
+#elif (defined __MINGW32__ || defined __GLIBC__) |
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.
@ederc you might want to apply this patch to your code
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.
@fingolfin Thanks, done.
I still try to understand your CPPFLAGS
solution for gmp.h. msolve
is a plain C project and I cannot see where these flags are set at all.
Something weird is going on with the CPU-detection, on x86_64-linux-gnu the log contains:
but on x86_64-linux-musl there is
which is then used during the compilation and at the end the auditor complains:
Maybe the checks only succeed when run natively on musl but fail everywhere else and then default to no? |
We should be able to run foreign executables for x86_64-linux-gnu |
Which might even make sense to set globally for all autoconf based packages, but could also break some build scripts... |
@fingolfin good to go? |
@giordano I think @ederc should decide that, it's his package, I made this PR to help him. The issues @benlorenz pointed out should be resolved, too, but that could be done in a future PR: in general, @ederc should think carefully about how to handle this kind of feature detection, given that we are building binaries here that are too be used on many different platforms, and so e.g. building a binary using AVX2 might not be the best idea, unless the plan really is to restrict to systems supporting it. Then again, I am not sure the results of those tests are all used? If they aren't, perhaps they could be disabled for now? |
From my point of view we could take this PR now. About the feature detection: This is crucial when it comes to performance, for example, AVX2 enabled gives you a factor of 3. Isn't there a way to provide binaries with AVX2 and without AVX2 for all supported platforms? |
Yes one can do that, using "microarchitectures". Indeed there is a function |
I am not quite sure how to use microarchs corectly, though. Maybe @giordano or @staticfloat can advise? |
Yes, I don't think there is any package currently using microarchitectures, but this looks like a potentially perfect use case. However this also means we haven't tested that into the wild, so there might be some things to iron out. I'm keeping a list of packages that could benefit from it in #2209, but the reason why it isn't much used it is because most libraries actually do runtime detection of CPU features (using for example CPUID), which is also more cross-compilation-friendly. |
Can we take this PR now? I think the other points we discussed need to be resolved in some future PRs. |
Yes, please |
Dependency(PackageSpec(name="FLINT_jll"), compat = "~200.800") | ||
Dependency(PackageSpec(name="MPFR_jll", uuid="3a97d323-0669-5f0c-9066-3539efd106a3")) | ||
Dependency("GMP_jll", v"6.2.0"), | ||
Dependency("FLINT_jll", compat = "~200.800.101"), |
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.
Uhm, this is changing the compat with FLINT_jll, I believe the registry won't like this within a build version. Is there a newer version of msolve to build to make this transition smoother?
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, we can bump the version to 0.1.5 using msolve commit 42137aaa68ac6077d25f7cb8e72ff01ac6f14f41 .
@fingolfin This commit of msolve already includes the changes provided by your sort_r.patch
, so I think we can remove the atomic_patch
part, right?
Trying to help @ederc expand this JLL: the main goal is to enable OpenMP on Linux and add Apple M1 support. However, to get started, I also enabled all platforms, just to see what all breaks, and how ;-). As such, this is of course just a draft for now.