Adjust OpenMP flags for macOS, weil_polynomials OpenMP support via meson#41626
Adjust OpenMP flags for macOS, weil_polynomials OpenMP support via meson#41626dimpase wants to merge 21 commits into
Conversation
|
Documentation preview for this PR (built with commit a338058; changes) is ready! 🎉 |
dimpase
changed the title
dimpase
requested review from
davidlowryduda
and removed request for
davidlowryduda
tobiasdiez
left a comment
tobiasdiez
left a comment
There was a problem hiding this comment.
Apart from trivial whitespace issues, the changes to the meson files look good to me. I cannot test if they do the job though as I don't have a mac.
dimpase
force-pushed
the
dimpase-patch-7
branch
from
0136fb4 to
177b9eb
Compare
|
even with this PR, compilation fails for me on macOS 26.3 A failing doctest indicates that some commits are missing https://github.com/sagemath/sage/actions/runs/21930838760/job/63334515946?pr=41626 |
it would have worked if installed fflas-ffpack etc from Homebree Macaulay2 tap... |
|
I did I first see and then and finally in the summary |
|
CI still fails due to problems with the formatting of the meson build. Could you please call the update-meson script to correct this? |
oops, this needs #40397 - otherwise openblas, linked on Homebrew with GNU OpenMP, conflicts with the "normal" OpenMP... |
|
need to modify the line And need to figure out how to build openblas on macOS with libomp rather than libgomp. |
|
This should work better, but still there are caveats on macOS - a possibility that one of the libraries uses OpenBLAS linked to GNU openMP (libgomp), but also llvm's OpenMP (libomp). The result is a bit problem... Actually the latest openblas ftom Homebrew links to openmp, not gopenmp. |
@tobiasdiez I've tried it, but it produces a huge diff for some reason. |
|
@tobiasdiez - the latest commit illustrates that something is broken in the update-meson script. It has added a non-existent file to The latter is a generated file, it absolutely should not be in |
dimpase
force-pushed
the
dimpase-patch-7
branch
from
b91b221 to
979ba2d
Compare
|
on conda macos CI we see or, in full: not sure if this is Conda-only. Maybe it comes from in |
|
@tobiasdiez we need to have a Homebrew-based CI run, as far as I understand. Conda uses its own clang, etc. I asked here: conda-forge/openblas-feedstock#182 |
|
Anyhow, I do not know now what OpenMP flags are needed for Conda on macOS. With or without |
|
Well, actually, with corrected autoconf macro, we don't need to hardcode OpenMP flags in |
| @@ -31,8 +31,14 @@ conf_data.set('SAGE_ARCHFLAGS', 'unset') | |||
| conf_data.set('SAGE_PKG_CONFIG_PATH', '') | |||
| openmp = dependency('openmp', required: false, disabler: true) | |||
| if openmp.found() | |||
There was a problem hiding this comment.
@jhpalmieri - can you try removing the whole if block here and see if the build works?
That's cause Meson probably is doing the right thing here without us messing up with flags.
There was a problem hiding this comment.
@jhpalmieri - can you try removing the whole
ifblock here and see if the build works?That's cause Meson probably is doing the right thing here without us messing up with flags.
I tried this:
diff --git a/src/sage/meson.build b/src/sage/meson.build
index d2edf7b687..4018fbf914 100644
--- a/src/sage/meson.build
+++ b/src/sage/meson.build
@@ -30,16 +30,16 @@ conf_data.set('SAGE_ARCHFLAGS', 'unset')
# not needed when using conda, as we then don't build any pc files
conf_data.set('SAGE_PKG_CONFIG_PATH', '')
openmp = dependency('openmp', required: false, disabler: true)
-if openmp.found()
- if host_machine.system() == 'darwin'
- conf_data.set('OPENMP_CFLAGS', '-Xpreprocessor -fopenmp')
- conf_data.set('OPENMP_CXXFLAGS', '-Xpreprocessor -fopenmp')
- conf_data.set('OPENMP_CLIB', '-lomp')
- else
- conf_data.set('OPENMP_CFLAGS', '-fopenmp')
- conf_data.set('OPENMP_CXXFLAGS', '-fopenmp')
- endif
-endif
+# if openmp.found()
+# if host_machine.system() == 'darwin'
+# conf_data.set('OPENMP_CFLAGS', '-Xpreprocessor -fopenmp')
+# conf_data.set('OPENMP_CXXFLAGS', '-Xpreprocessor -fopenmp')
+# conf_data.set('OPENMP_CLIB', '-lomp')
+# else
+# conf_data.set('OPENMP_CFLAGS', '-fopenmp')
+# conf_data.set('OPENMP_CXXFLAGS', '-fopenmp')
+# endif
+# endif
gap_exe = find_program('gap', required: not is_windows, disabler: true)
if gap_exe.found()
gaprun = run_command(and it made no difference.
There was a problem hiding this comment.
Thanks. Looks like that we either are converging on a Meson bug, or on a fix :-)
Could you attach here build/sage-distro/meson-info/intro-dependencies.json from the latest run?
On top of the latter diff (in #41626 (comment)), could you please try applying
--- a/build/pkgs/sagelib/spkg-install.in
+++ b/build/pkgs/sagelib/spkg-install.in
@@ -31,6 +31,9 @@ export SAGE_SRC_ROOT=/doesnotexist
export SAGE_DOC_SRC=/doesnotexist
export SAGE_DOCS=/doesnotexist
+unset OPENMP_CFLAGS
+unset OPENMP_CXXFLAGS
+
# We also poison all directories below SAGE_LOCAL
# except for SAGE_SPKG_SCRIPTS, which is needed by sage-dist-helpers
export SAGE_PKGCONFIG=/doesnotexistand rebuilding? The latter should be removing any remaining interference we might have over Meson's dealing with OpenMP.
There was a problem hiding this comment.
This made no difference: I still see "error: unknown argument: '-MD'".
There was a problem hiding this comment.
OK, so this seems to be a bug in Meson dealing with OpenMP: flags containing -Xpreprocessor are received from pkg-config files, and not dealt with properly
But we need a small reproducer, to provide a good bug report
There was a problem hiding this comment.
@jhpalmieri - could you please post
intro-dependencies.jsonfor the--with-darwin-accerate=yescase? I really would like to see how-framework Accelerateappear there.
Here it is.
There was a problem hiding this comment.
Thanks. Here is the bug report: mesonbuild/meson#15764
There was a problem hiding this comment.
Does it work if you use -Xpreprocessor=-fopenmp instead? (from the linked meson issue)
There was a problem hiding this comment.
Meson upstream acknowledged this as a regression, and aims to fix it in the next release, 1.11.2. Which should be in a couple of weeks, so I'd wait for it before proceeding here
There was a problem hiding this comment.
Does it work if you use
-Xpreprocessor=-fopenmpinstead? (from the linked meson issue)
it's not documented (perhaps they talked about using the = padding in their code, not as input)?
There is a rough equivalent, -Wp,-fopenmp - but I'm not sure whether it would be understood by our build system (probably not out of the box)
dimpase
force-pushed
the
dimpase-patch-7
branch
2 times, most recently
from
840058f to
e1724f1
Compare
Removed comments regarding OpenMP support.
Co-authored-by: Tobias Diez <code@tobiasdiez.com> Update src/sage/meson.build Co-authored-by: Tobias Diez <code@tobiasdiez.com> Update src/sage/meson.build Co-authored-by: Tobias Diez <code@tobiasdiez.com>
Note that we disabled checking for "-openmp" flag, as it leads to creation of an executable called "penmp" during the ./configure run
WIP patch for libtool to allow "-Xpreprocessor -fopenmp" same treatment for linbox as for fflas WIP: same libtool patch as for fflas
for an unclear so far reason, this needs to be explicit
Link libomp after installation in CI workflow.
This reverts commit 8daa4d9.
8 participants
Should fix a problem reported on #40898 - OpenMP on macOS needs different flags
Also, correctly support OpenMP for weil_polynomials with Meson
📝 Checklist
⌛ Dependencies