Opened 11 months ago

Closed 8 months ago

#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: tscrim, mkoeppe, fbissey, gh-kliem, 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:

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

  • Status changed from new to needs_review

comment:2 Changed 11 months ago by tscrim

  • Cc fbissey added

comment:3 follow-up: Changed 11 months ago by 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?

comment:4 follow-up: Changed 11 months ago by 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()?

comment:5 Changed 11 months ago by git

  • Commit changed from b76dc3ccf4bbc2715754a9ae2e91bda22e01e54a to a9d3dfed7bf31b9167f5af553751b19a506b4c22

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

a9d3dfesplit to account for multiple arguments

comment:6 in reply to: ↑ 4 Changed 11 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 11 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 11 months ago by fbissey

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 gh-kliem

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

comment:10 Changed 10 months ago by mkoeppe

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 10 months ago by git

  • Commit changed from a9d3dfed7bf31b9167f5af553751b19a506b4c22 to 7b3da2185c0a8c4cc383f43fdcc39ded6546e37d

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 10 months ago by mkoeppe

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

comment:13 follow-up: Changed 10 months ago by mkoeppe

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 mkoeppe

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 10 months ago by git

  • Commit changed from 7b3da2185c0a8c4cc383f43fdcc39ded6546e37d to 7d3f2307d4cc90b1069a36a6ad93c3b57802fd3b

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

927291fsimplifications
d71c54aset default
7d3f230more solid check

comment:16 follow-up: Changed 10 months ago by 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?

comment:17 Changed 10 months ago by mkoeppe

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

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

comment:20 Changed 10 months ago by mkoeppe

  • Cc Winfried added

comment:21 Changed 10 months ago by gh-kliem

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

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

comment:22 Changed 10 months ago by mkoeppe

  • 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 gh-kliem

Thank you.

comment:24 Changed 10 months ago by git

  • 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:

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

  • Dependencies set to #31384
  • Status changed from needs_review to positive_review

Merged in #31384 as requested.

comment:26 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-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 8 months ago by vbraun

  • Branch changed from u/gh-kliem/check_openmp_at_configuration to c3cc31e29ca6c8c74aec619e422858d97f92e1c5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.