Opened 11 months ago
Closed 8 months ago
#31499 closed enhancement (fixed)
Check OpenMP at configuration
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  cython  Keywords:  openmp, cython, parallel 
Cc:  tscrim, mkoeppe, fbissey, ghkliem, Winfried  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Matthias Koeppe 
Report Upstream:  N/A  Work issues:  
Branch:  c3cc31e (Commits, GitHub, GitLab)  Commit:  c3cc31e29ca6c8c74aec619e422858d97f92e1c5 
Dependencies:  #31384  Stopgaps: 
Description
We define cython aliases OPENMP_CFLAGS
and OPENMP_CXXFLAGS
.
They must be used as extra_link_args
and extra_compile_args
to get cython.parallel
to work in parallel.
If they are empty, cython.parallel.prange
will still compile and work (but not in parallel). This is useful in case that OpenMP isn't supported, see e.g. https://github.com/kliem/sage/pull/40/checks?check_run_id=2076988665).
With this ticket we can just run
sage: cython(''' ....: #distutils: extra_compile_args = OPENMP_CFLAGS ....: #distutils: extra_link_args = OPENMP_CFLAGS ....: from cython.parallel import prange ....: ....: cdef int i ....: cdef int n = 30 ....: cdef int sum = 0 ....: ....: for i in prange(n, num_threads=4, nogil=True): ....: sum += i ....: ....: print(sum) ....: ''')
regardless of whether the current system actually supports OpenMP.
Change History (27)
comment:1 Changed 11 months ago by
 Status changed from new to needs_review
comment:2 Changed 11 months ago by
 Cc fbissey added
comment:3 followup: ↓ 7 Changed 11 months ago by
comment:4 followup: ↓ 6 Changed 11 months ago by
+ # OpenMP + aliases["OPENMP_CFLAGS"] = [SAGE_OPENMP_CFLAGS] if SAGE_OPENMP_CFLAGS else [] + aliases["OPENMP_CXXFLAGS"] = [SAGE_OPENMP_CXXFLAGS] if SAGE_OPENMP_CXXFLAGS else [] +
As SAGE_OPENMP_CFLAGS
could probably consist of several arguments separated by space, perhaps this should be aliases["OPENMP_CFLAGS"] = SAGE_OPENMP_CFLAGS.split()
?
comment:5 Changed 11 months ago by
 Commit changed from b76dc3ccf4bbc2715754a9ae2e91bda22e01e54a to a9d3dfed7bf31b9167f5af553751b19a506b4c22
Branch pushed to git repo; I updated commit sha1. New commits:
a9d3dfe  split to account for multiple arguments

comment:6 in reply to: ↑ 4 Changed 11 months ago by
Replying to mkoeppe:
+ # OpenMP + aliases["OPENMP_CFLAGS"] = [SAGE_OPENMP_CFLAGS] if SAGE_OPENMP_CFLAGS else [] + aliases["OPENMP_CXXFLAGS"] = [SAGE_OPENMP_CXXFLAGS] if SAGE_OPENMP_CXXFLAGS else [] +As
SAGE_OPENMP_CFLAGS
could probably consist of several arguments separated by space, perhaps this should bealiases["OPENMP_CFLAGS"] = SAGE_OPENMP_CFLAGS.split()
?
Good point.
comment:7 in reply to: ↑ 3 Changed 11 months ago by
Replying to fbissey:
Seems fair. Why is the extra test in
python3/spkgconfigure
needed? Is it to deal with case where the compiler used to build python is not the same as the rest of sage?
Exactly. In #27122 I didn't consider this and then we had to solve #30725. This was the solution we came up with (just reject OpenMP alltogether, if we cannot find consistent arguments to make it work.)
We don't try as hard as we could (if sage uses gcc, but for cython clang, it still might be possible to enable OpenMP for cython with different arguments). But this solution here should enable OpenMP in cython on many platforms and still work on the others.
comment:8 Changed 11 months ago by
Argh!!! I was going to answer that  hours ago  but I got interrupted by my day work (which is currently horrible, 2021 so far is worse for me than the whole of 2020).
ax_cv_[]_AC_LANG_ABBREV[]_openmp=unknown # Flags to try: fopenmp (gcc), mp (SGI & PGI), # qopenmp (icc>=15), openmp (icc), # xopenmp (Sun), omp (Tru64), # qsmp=omp (AIX), # none
As you can see there is only a single switch in each case which tells the compiler what headers and macro to enable at compile time and what library to use at link time. The greatest danger usually is that people, or build systems, forget you should also include the CFLAGS at linking time. Very often lgomp
or similar is missing because of that oversight.
I guess that proof things in case things change in the future (most likely several years).
comment:9 Changed 10 months ago by
 Cc ghkliem added
 Reviewers set to https://github.com/kliem/sage/pull/43/checks
comment:10 Changed 10 months ago by
It would be better to set the new variables in sage_conf.py.in
, not sageenvconfig.in
, because they are not used as environment variables. See comments at the top of these files.
And I think you can get rid of SAGE_HAVE_OPENMP
completely  in build/pkgs/python3/spkgconfigure.m4
just check whether the flags are nonempty.
comment:11 Changed 10 months ago by
 Commit changed from a9d3dfed7bf31b9167f5af553751b19a506b4c22 to 7b3da2185c0a8c4cc383f43fdcc39ded6546e37d
comment:12 Changed 10 months ago by
I guess that should be AS_IF([test n "$OPENMP_CFLAGS$OPENMP_CXXFLAGS"]
(test both)
comment:13 followup: ↓ 14 Changed 10 months ago by
And SAGE_OPENMP_CFLAGS.split() if SAGE_OPENMP_CFLAGS else []
can be simplified to just SAGE_OPENMP_CFLAGS.split()
comment:14 in reply to: ↑ 13 Changed 10 months ago by
Replying to mkoeppe:
And
SAGE_OPENMP_CFLAGS.split() if SAGE_OPENMP_CFLAGS else []
can be simplified to justSAGE_OPENMP_CFLAGS.split()
... if you set a default of ""
in env.py
comment:15 Changed 10 months ago by
 Commit changed from 7b3da2185c0a8c4cc383f43fdcc39ded6546e37d to 7d3f2307d4cc90b1069a36a6ad93c3b57802fd3b
comment:16 followup: ↓ 18 Changed 10 months ago by
On macOS (using /usr/bin/gcc) I am getting
checking whether C++ compiler accepts "march=native"... yes checking for OpenMP flag of C compiler... unknown checking for OpenMP flag of C++ compiler... unknown
is this expected?
comment:17 Changed 10 months ago by
Also, best to merge #31384 which touches the same function and will cause a merge conflict
comment:18 in reply to: ↑ 16 Changed 10 months ago by
Replying to mkoeppe:
On macOS (using /usr/bin/gcc) I am getting
checking whether C++ compiler accepts "march=native"... yes checking for OpenMP flag of C compiler... unknown checking for OpenMP flag of C++ compiler... unknownis this expected?
macOS might be a bit more complicated. https://stackoverflow.com/questions/66040039/openmpsupportformacusingclangorgcc
I'm happy to add whatever to the list to make it work. Maybe Xclang fopenmp
will work for you?
comment:19 Changed 10 months ago by
I usually take the example from src/sage/env.py
to check whether it works.
comment:20 Changed 10 months ago by
 Cc Winfried added
comment:21 Changed 10 months ago by
How about fopenmp=libgomp
? Found this is in normaliz (but it is commented out).
comment:22 Changed 10 months ago by
 Reviewers changed from https://github.com/kliem/sage/pull/43/checks to Matthias Koeppe
 Status changed from needs_review to positive_review
Let's just take this to mean that Xcode's gcc can't do OpenMP. Good enough for this ticket.
comment:23 Changed 10 months ago by
Thank you.
comment:24 Changed 10 months ago by
 Commit changed from 7d3f2307d4cc90b1069a36a6ad93c3b57802fd3b to c3cc31e29ca6c8c74aec619e422858d97f92e1c5
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
4f42440  sage.env.cython_aliases: Do not fail if one of the listed libraries is not known to pkgconfig

3cb8f90  sage.env.cython_aliases: Make module lapack optional; generalize the interface

c3cc31e  merge in 31384

comment:25 Changed 10 months ago by
 Dependencies set to #31384
 Status changed from needs_review to positive_review
Merged in #31384 as requested.
comment:26 Changed 10 months ago by
 Milestone changed from sage9.3 to sage9.4
Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review.
comment:27 Changed 8 months ago by
 Branch changed from u/ghkliem/check_openmp_at_configuration to c3cc31e29ca6c8c74aec619e422858d97f92e1c5
 Resolution set to fixed
 Status changed from positive_review to closed
Seems fair. Why is the extra test in
python3/spkgconfigure
needed? Is it to deal with case where the compiler used to build python is not the same as the rest of sage?