#31499 closed enhancement (fixed)

Check OpenMP at configuration

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-9.4
Component: cython Keywords: openmp, cython, parallel
Cc: Travis Scrimshaw, Matthias Köppe, François Bissey, gh-kliem, Winfried Bruns Merged in:
Authors: Jonathan Kliem Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: c3cc31e (Commits, GitHub, GitLab) Commit: c3cc31e29ca6c8c74aec619e422858d97f92e1c5
Dependencies: #31384 Stopgaps:

Status badges

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 21 months ago by gh-kliem

Status: newneeds_review

comment:2 Changed 21 months ago by Travis Scrimshaw

Cc: François Bissey added

comment:3 Changed 21 months ago by François Bissey

Seems fair. Why is the extra test in python3/spkg-configure needed? Is it to deal with case where the compiler used to build python is not the same as the rest of sage?

comment:4 Changed 21 months ago by Matthias Köppe

+    # 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 21 months ago by git

Commit: b76dc3ccf4bbc2715754a9ae2e91bda22e01e54aa9d3dfed7bf31b9167f5af553751b19a506b4c22

Branch pushed to git repo; I updated commit sha1. New commits:

a9d3dfesplit to account for multiple arguments

comment:6 in reply to:  4 Changed 21 months ago by gh-kliem

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 be aliases["OPENMP_CFLAGS"] = SAGE_OPENMP_CFLAGS.split()?

Good point.

comment:7 in reply to:  3 Changed 21 months ago by gh-kliem

Replying to fbissey:

Seems fair. Why is the extra test in python3/spkg-configure 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 21 months ago by François Bissey

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 21 months ago by gh-kliem

Cc: gh-kliem added
Reviewers: https://github.com/kliem/sage/pull/43/checks

comment:10 Changed 21 months ago by Matthias Köppe

It would be better to set the new variables in sage_conf.py.in, not sage-env-config.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/spkg-configure.m4 just check whether the flags are nonempty.

comment:11 Changed 21 months ago by git

Commit: a9d3dfed7bf31b9167f5af553751b19a506b4c227b3da2185c0a8c4cc383f43fdcc39ded6546e37d

Branch pushed to git repo; I updated commit sha1. New commits:

3e415ceMerge branch 'u/gh-kliem/check_openmp_at_configuration' of git://trac.sagemath.org/sage into u/gh-kliem/check_openmp_at_configuration_reb
499af5dremove SAGE_HAVE_OPENMP
7b3da21move to sage_conf.py.in

comment:12 Changed 21 months ago by Matthias Köppe

I guess that should be AS_IF([test -n "$OPENMP_CFLAGS$OPENMP_CXXFLAGS"] (test both)

comment:13 Changed 21 months ago by Matthias Köppe

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 21 months ago by Matthias Köppe

Replying to mkoeppe:

And SAGE_OPENMP_CFLAGS.split() if SAGE_OPENMP_CFLAGS else [] can be simplified to just SAGE_OPENMP_CFLAGS.split()

... if you set a default of "" in env.py

comment:15 Changed 21 months ago by git

Commit: 7b3da2185c0a8c4cc383f43fdcc39ded6546e37d7d3f2307d4cc90b1069a36a6ad93c3b57802fd3b

Branch pushed to git repo; I updated commit sha1. New commits:

927291fsimplifications
d71c54aset default
7d3f230more solid check

comment:16 Changed 21 months ago by Matthias Köppe

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 21 months ago by Matthias Köppe

Also, best to merge #31384 which touches the same function and will cause a merge conflict

comment:18 in reply to:  16 Changed 21 months ago by gh-kliem

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... unknown

is this expected?

macOS might be a bit more complicated. https://stackoverflow.com/questions/66040039/openmp-support-for-mac-using-clang-or-gcc

I'm happy to add whatever to the list to make it work. Maybe -Xclang -fopenmp will work for you?

comment:19 Changed 21 months ago by gh-kliem

I usually take the example from src/sage/env.py to check whether it works.

comment:20 Changed 21 months ago by Matthias Köppe

Cc: Winfried Bruns added

comment:21 Changed 21 months ago by gh-kliem

How about -fopenmp=libgomp? Found this is in normaliz (but it is commented out).

Last edited 21 months ago by gh-kliem (previous) (diff)

comment:22 Changed 21 months ago by Matthias Köppe

Reviewers: https://github.com/kliem/sage/pull/43/checksMatthias Koeppe
Status: needs_reviewpositive_review

Let's just take this to mean that Xcode's gcc can't do OpenMP. Good enough for this ticket.

comment:23 Changed 21 months ago by gh-kliem

Thank you.

comment:24 Changed 21 months ago by git

Commit: 7d3f2307d4cc90b1069a36a6ad93c3b57802fd3bc3cc31e29ca6c8c74aec619e422858d97f92e1c5
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

4f42440sage.env.cython_aliases: Do not fail if one of the listed libraries is not known to pkgconfig
3cb8f90sage.env.cython_aliases: Make module lapack optional; generalize the interface
c3cc31emerge in 31384

comment:25 Changed 21 months ago by gh-kliem

Dependencies: #31384
Status: needs_reviewpositive_review

Merged in #31384 as requested.

comment:26 Changed 20 months ago by Matthias Köppe

Milestone: sage-9.3sage-9.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 19 months ago by Volker Braun

Branch: u/gh-kliem/check_openmp_at_configurationc3cc31e29ca6c8c74aec619e422858d97f92e1c5
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.