Support Apple clang specific compilation/linking flags to enable OpenMP #42

gdurif · 2022-04-01T08:27:37Z

Potential fix for #40

Apple clang requires -Xpreprocessor -fopenmp compile flags (instead of just -fopenmp) and -lomp linking flags (instead of -fopenmp) to support and enable OpenMp.

Work in progress, I am still not sure whether it works or not on MacOS.

Edit: should be ready c.f. #42 (comment)

gdurif · 2022-04-01T08:41:20Z

Log 2022/04/01 1040: remaining problem

On my MacOS VM, after installing libomp with brew install libomp, I can compile the following code:

#include <omp.h>
#include <stdio.h>
int main() {
#pragma omp parallel
printf("Hello from thread %d over %d\n", omp_get_thread_num(), omp_get_num_threads());
}

with clang -Xpreprocessor -fopenmp test_openmp.c -lomp and the program works in parallel.

However, when I try to do the following:

from setuptools import Extension
from extension_helpers import add_openmp_flags_if_available
# create a dummy extension
dummy_ext = Extension(
    'dummy_ext',
    sources=['dummy.cpp'],
    extra_compile_args=['-DNDEBUG', '-DUSE_BLAS_LIB', '-std=c++11'], 
    language='c++',
    depends=['dummy.h']
)
# add OpenMP flag if available
omp_support = add_openmp_flags_if_available(dummy_ext)

I get the following

clang: warning: argument unused during compilation: '-Xpreprocessor -fopenmp' [-Wunused-command-line-argument]
test_openmp.c:2:10: fatal error: 'omp.h' file not found

which I don't understand.

gdurif · 2022-04-01T10:14:51Z

Log 2022/04/01 1215: moving a bit forward

Running:

from setuptools.command.build_ext import new_compiler, customize_compiler
from extension_helpers._openmp_helpers import get_openmp_flags
import distutils.log

distutils.log.set_verbosity(1)

ccompiler = new_compiler()
customize_compiler(ccompiler)

openmp_flags = get_openmp_flags()
compile_flags = openmp_flags.get('compiler_flags')

ccompiler.compile(['test_openmp.c'], extra_postargs=compile_flags)

gives the following compilation command:

clang -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Tk.framework/Versions/8.5/Headers -c test_openmp.c -o test_openmp.o -I/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include -Xpreprocessor -fopenmp

with the error:

clang: warning: argument unused during compilation: '-Xpreprocessor -fopenmp' [-Wunused-command-line-argument]
test_openmp.c:1:10: fatal error: 'omp.h' file not found

If I move -Xpreprocessor -fopenmp at the beginning of the command and remove -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk, it now works.

Thus:

OpenMP flags should be extra_preargs and not extra_postargs (c.f. compile doc)
It only remains to find a way to make libomp available within /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk (big ??) -> to investigate what is -isysroot

Edit: second point solved by

export CFLAGS="-I/usr/local/opt/libomp/include"
export LDFLAGS="-L/usr/local/opt/libomp/lib"

-> a warning should be added

…ude and lib path (if not aleady setup) for Apple Clang

…sor -fopenmp' not undertood by Apple Clang as postargs but ok as preargs)

gdurif · 2022-04-01T12:32:35Z

Should be ready to merge.

Summary of modifications

add a new function that check if compiler is 'Apple Clang' (namely check_apple_clang() in extension_helpers/_setup_helpers.py
use the function check_apple_clang in get_openmp_flags() (defined in extension_helpers/_openmp_helpers.py) to assess if the user is using Apple Clang
When using Apple Clang compiler, again in get_openmp_flags(), a warning about making OpenMP source and library findable for the compilation and linking is raised. In addition, if CFLAGS (resp. LDFLAGS) is not set, standard OpenMP path are tried
in the function check_openmp_support() (defined in extension_helpers/_openmp_helpers.py), compile_args (resp. link_args) are passed to the compiler (resp. linker) as extra_preargs instead of extra_postargs (c.f, doc for distutils.ccompiler.CCompiler.compile() method) because the -Xprocessor -fopenmp flags was not understood by Apple Clang as extra_postargs.
The new function check_apple_clang() is tested in extension_helpers/tests/test_setup_helpers.py.

Edit

_get_flag_value_from_var() (defined in extension_helpers/_openmp_helpers.py) is now able to extract multiple flags if corresponding environment variable contains more than one. It now returns a list of flag (and the function _get_flag_value_from_var() is now tested).

End of edit

Remaining issue

on my MacOS 10.15 VM, running^[1] add_openmp_flags_if_available() raise a clang warning clang: warning: argument unused during compilation: '-Xpreprocessor -fopenmp' [-Wunused-command-line-argument] but returns a True value meaning that OpenMP is enabled (note that without -Xpreprocessor, I get an error unknown -fopenmp so it is necessary). Strange thing is that if I directly run in a terminal the exact same compilation command^[2] used by distutils.ccompiler.CCompiler.compile(), I do not get the clang warning.

[1] here is the code used:

from setuptools import Extension
from pprint import pprint
from extension_helpers import add_openmp_flags_if_available
import distutils.log

distutils.log.set_verbosity(1)

# create a dummy extension
dummy_ext = Extension(
    'dummy_ext',
    sources=['dummy.cpp'],
    extra_compile_args=['-DNDEBUG', '-DUSE_BLAS_LIB', '-std=c++11'], 
    language='c++',
    depends=['dummy.h']
)

# check extension
print("-- Check extension")
pprint(dummy_ext.__dict__)

# add OpenMP flag if available
omp_support = add_openmp_flags_if_available(dummy_ext)

# check OpenMP support
print(f"-- OpenMP support: {omp_support}")

# check extension
print("-- Check extension")
pprint(dummy_ext.__dict__)

[2] The compile command is:

clang -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX11.sdk -Xpreprocessor -fopenmp -I/usr/local/opt/libomp/include -c test_openmp.c -o objects/test_openmp.o

It is printed by running the code above because of distutils.log.set_verbosity(1).

lukeolson · 2022-04-01T13:28:19Z

extension_helpers/_openmp_helpers.py

+            )
+            log.warn(msg)
+            compile_flags.append('-Xpreprocessor -fopenmp')
+            if not 'CFLAG' in os.environ and \


Should this be CFLAGS (and LDFLAGS below)?

Yes, thanks! I fixed it in 094b820

lukeolson · 2022-04-01T13:28:19Z

extension_helpers/_openmp_helpers.py

+            )
+            log.warn(msg)
+            compile_flags.append('-Xpreprocessor -fopenmp')
+            if not 'CFLAG' in os.environ and \


Should this be CFLAGS (and LDFLAGS below)?

Yes, thanks! I fixed it in 094b820

gdurif · 2022-04-01T15:02:02Z

@lukeolson Seeing your comment #40 (comment) made me think that the code should also check for CXXFLAGS.

Edit: introduced by dcca6d7

…corresponding environment variable contains one or more

lukeolson · 2022-04-01T15:38:14Z

@lukeolson Seeing your comment #40 (comment) made me think that the code should also check for CXXFLAGS.

Edit: introduced by dcca6d7

Great! For flags I followed scikit-learn

I'll test this with their conda steps too.

gdurif · 2022-04-01T15:39:49Z

Note: I also modified (c.f e4c01f8 ) _get_flag_value_from_var() (defined in extension_helpers/_openmp_helpers.py) that is now able to extract multiple flags if corresponding environment variable contains more than one. It now returns a list of flag.

Example assuming CFLAGS="-I/path/to/file1 -I/path/to/file2 -custom_option1 -custom_option2":

>>> _get_flag_value_from_var('-I', 'CFLAGS')
['/path/to/file1', '/path/to/file2']

Until now, it was only returning the first occurrence.

Note: I also added a test for _get_flag_value_from_var().

lukeolson · 2022-04-06T21:48:52Z

extension_helpers/_openmp_helpers.py

-
-        compile_flags.append('-fopenmp')
-        link_flags.append('-fopenmp')
+            for _ in include_path:


Should this be lib_path? Right now I'm not picking up any LDFLAGS from the environment.

Yes, it is. Thanks. Fixed by e93d695

lukeolson · 2022-04-06T21:52:13Z

I left a note on the path above... likely a minor issue.

Another possible issue: /usr/local/opt/libomp/include and /usr/local/opt/libomp/lib are for Intel-based macs (https://docs.brew.sh/Installation, "/usr/local for macOS Intel, /opt/homebrew for Apple Silicon and /home/linuxbrew/.linuxbrew for Linux"). One option is to look at $(brew --prefix libomp). Another option is to only rely on the environment variables. Thoughts?

gdurif · 2022-04-11T12:54:42Z

@lukeolson I am not very familiar with MacOS, can we expect brew to be a standard tool on MacOS (that will be always available) ? so that a call to brew --prefix libomp is likely to work on any MacOS system and whaterver the version ?

Thanks

samuelstjean · 2022-04-11T13:14:34Z

probably not, it's a third party tool that provides a community managed kind of repository. It is largely already installed on most buildbots we use though since it's the only sane way to install third party stuff it seems. Regular people probably won't have it out of the box installed because of that I'd say.

gdurif · 2022-04-11T13:46:11Z

Thanks @samuelstjean, I guess I will have to do a try...except calling brew and if not possible append standard path.

lukeolson · 2022-04-11T22:00:03Z

Taking a look at cmake [1], one options is to simply encode the right base path -- in this case /opt/homebrew: https://github.com/Kitware/CMake/blob/master/Modules/Platform/Darwin.cmake#L247

With homebrew everything is symlinked in /opt/homebrew/lib and /opt/homebrew/include, so we don't need the call to brew (as I initially suggested). Simply listing some paths will suffice, I think. And environment variables would be preferred.

Anyway thanks for your work on this! I'll give this version a test.

[1] https://github.com/Kitware/CMake/blob/master/Modules/FindOpenMP.cmake

gdurif · 2022-04-12T13:11:43Z

@lukeolson Thanks for the feedback.

It appears that brew/OpenMP configuration does not seem to be standard on every machine. On the two MacOS VMs (10.15.4 and 11.6) that I have access to, /opt/homebrew does not exist.

libomp can also be installed in /usr/local/opt/libomp on macOS Monterey and the new Apple M1 chip, (c.f. here), like on any Intel architecture.

My guess is I will try to cover different possibilities with a combination of brew --prefix libomp call and check for libomp availability in /usr/local/opt/libomp or in /opt/homebrew/lib and /opt/homebrew/include.

As a last resources, the user can setup CFLAGS and LDFLAGS variables to override this anyway (and there is a warning telling this).

MuellerSeb · 2022-06-01T10:09:38Z

Would this also work in a conda environment on MacOS?

gdurif · 2022-06-02T05:45:46Z

@MuellerSeb I don't know, I guess it should but I did not test it.

gdurif added 4 commits April 1, 2022 09:50

function to check if current compiler is Apple Clang

200406c

test function checking if current compiler is Apple Clang

b4ea694

specific compilation and linking flags for Apple Clang compiler

45b198c

add comment about libomp + refine except statement

40a39fd

This was referenced Apr 1, 2022

clang fopenmp on macOS #40

Open

Using multiple threads/cores on OSX getspams/spams-python#18

Open

gdurif added 4 commits April 1, 2022 13:55

specific warning message for Apple Clang users + specific OpenMP incl…

ac7644e

…ude and lib path (if not aleady setup) for Apple Clang

fix typo in dir check

401d869

add preargs instead of postargs to compile command (because '-Xproces…

ad2b191

…sor -fopenmp' not undertood by Apple Clang as postargs but ok as preargs)

fix typo regarding link_flags

31de7ec

lukeolson reviewed Apr 1, 2022

View reviewed changes

fix typo

094b820

gdurif added 2 commits April 1, 2022 17:08

account for potential CXXFLAGS env variable

dcca6d7

modify _get_flag_value_from_var to return list of extracted flags if …

e4c01f8

…corresponding environment variable contains one or more

lukeolson reviewed Apr 6, 2022

View reviewed changes

fix typo regarding lib path parsing

e93d695

check for the OS (i.e. MacOS) before testing for Apple Clang compiler

cc7aafd

gdurif added 2 commits April 12, 2022 15:31

improve libomp search on MacOS

b1a10a5

improve extraction of command output

f88cdbb

MuellerSeb mentioned this pull request May 27, 2022

Build: update setuptools config and CI GeoStat-Framework/GSTools#241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Apple clang specific compilation/linking flags to enable OpenMP #42

Support Apple clang specific compilation/linking flags to enable OpenMP #42

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 1, 2022 •

edited

lukeolson Apr 1, 2022

gdurif Apr 1, 2022

lukeolson Apr 1, 2022

gdurif Apr 1, 2022

gdurif commented Apr 1, 2022 •

edited

lukeolson commented Apr 1, 2022

gdurif commented Apr 1, 2022 •

edited

lukeolson Apr 6, 2022

gdurif Apr 11, 2022

lukeolson commented Apr 6, 2022

gdurif commented Apr 11, 2022

samuelstjean commented Apr 11, 2022

gdurif commented Apr 11, 2022

lukeolson commented Apr 11, 2022

gdurif commented Apr 12, 2022 •

edited

MuellerSeb commented Jun 1, 2022

gdurif commented Jun 2, 2022

Support Apple clang specific compilation/linking flags to enable OpenMP #42

Are you sure you want to change the base?

Support Apple clang specific compilation/linking flags to enable OpenMP #42

Conversation

gdurif commented Apr 1, 2022 • edited

gdurif commented Apr 1, 2022 • edited

gdurif commented Apr 1, 2022 • edited

gdurif commented Apr 1, 2022 • edited

Summary of modifications

Remaining issue

lukeolson Apr 1, 2022

Choose a reason for hiding this comment

gdurif Apr 1, 2022

Choose a reason for hiding this comment

lukeolson Apr 1, 2022

Choose a reason for hiding this comment

gdurif Apr 1, 2022

Choose a reason for hiding this comment

gdurif commented Apr 1, 2022 • edited

lukeolson commented Apr 1, 2022

gdurif commented Apr 1, 2022 • edited

lukeolson Apr 6, 2022

Choose a reason for hiding this comment

gdurif Apr 11, 2022

Choose a reason for hiding this comment

lukeolson commented Apr 6, 2022

gdurif commented Apr 11, 2022

samuelstjean commented Apr 11, 2022

gdurif commented Apr 11, 2022

lukeolson commented Apr 11, 2022

gdurif commented Apr 12, 2022 • edited

MuellerSeb commented Jun 1, 2022

gdurif commented Jun 2, 2022

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 1, 2022 •

edited

gdurif commented Apr 12, 2022 •

edited