Check OpenMP at configuration

[kliem]

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.

Depends on #31384

CC: @tscrim @mkoeppe @kiwifb @kliem @w-bruns

Component: cython

Keywords: openmp, parallel

Author: Jonathan Kliem

Branch/Commit: c3cc31e

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/31499

[kiwifb]

comment:3

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?

[mkoeppe]

comment:4

+    # 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()?

[sagetrac-git]

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

a9d3dfe

split to account for multiple arguments

[sagetrac-git]

Changed commit from b76dc3c to a9d3dfe

[kliem]

comment:6

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.

[kliem]

comment:7

Replying to @kiwifb:

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.

[kiwifb]

comment:8

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

[kliem]

Reviewer: https://github.com/kliem/sage/pull/43/checks

[mkoeppe]

comment:10

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.

[sagetrac-git]

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

3e415ce	Merge branch 'u/gh-kliem/check_openmp_at_configuration' of git://trac.sagemath.org/sage into u/gh-kliem/check_openmp_at_configuration_reb
499af5d	remove SAGE_HAVE_OPENMP
7b3da21	move to sage_conf.py.in

[sagetrac-git]

Changed commit from a9d3dfe to 7b3da21

[mkoeppe]

comment:12

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

[mkoeppe]

comment:13

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

[mkoeppe]

comment:14

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

[sagetrac-git]

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

927291f	simplifications
d71c54a	set default
7d3f230	more solid check

[sagetrac-git]

Changed commit from 7b3da21 to 7d3f230

[mkoeppe]

comment:16

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?

[mkoeppe]

comment:17

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

[kliem]

comment:18

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?

[kliem]

comment:19

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

[kliem]

comment:21

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

[mkoeppe]

comment:22

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

[mkoeppe]

Changed reviewer from https://github.com/kliem/sage/pull/43/checks to Matthias Koeppe

[kliem]

comment:23

Thank you.

[sagetrac-git]

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

[sagetrac-git]

Changed commit from 7d3f230 to c3cc31e

[kliem]

comment:25

Merged in #31384 as requested.

[kliem]

Dependencies: #31384

[mkoeppe]

comment:26

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review.

[vbraun]

Changed branch from u/gh-kliem/check_openmp_at_configuration to c3cc31e