-
Notifications
You must be signed in to change notification settings - Fork 998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support OpenMP with system macOS toolchain #4348
Comments
I think Jim Hester raised this same issue, let me see... I see, it was on the orgs page... good to have this in the issue tracker https://github.com/orgs/Rdatatable/teams/project-members/discussions/34 |
I am pasting my working version of
macOS Mojave, 10.14.6 |
Since @krivard last post was super helpful for me to get things working, here is my working version of # 2020-11-15 Added: -Xpreprocessor -fopenmp
CC=/usr/local/Cellar/llvm/11.0.0/bin/clang -Xpreprocessor -fopenmp
# 2020-11-15 Added: -Xpreprocessor -fopenmp -lomp -I/usr/local/include
CXX=/usr/local/Cellar/llvm/11.0.0/bin/clang++ -Xpreprocessor -fopenmp -lomp -I/usr/local/include
CXX11=/usr/local/Cellar/llvm/11.0.0/bin/clang++
CXX14=/usr/local/Cellar/llvm/11.0.0/bin/clang++
CXX17=/usr/local/Cellar/llvm/11.0.0/bin/clang++
CXX1X=/usr/local/Cellar/llvm/11.0.0/bin/clang++
# 2020-11-15 Added: CFLAGS
CFLAGS=-g -O3 -Wall -pedantic -std=gnu99 -mtune=native -pipe
# 2020-11-15 Added: CXXFLAGS
CXXFLAGS=-g -O3 -Wall -pedantic -std=c++11 -mtune=native -pipe
LDFLAGS=-L/usr/local/Cellar/llvm/11.0.0/lib
SHLIB_OPENMP_CFLAGS= -fopenmp
SHLIB_OPENMP_CXXFLAGS= -fopenmp Also Mac Mojave 10.14.6 Please reply if you find anything about this that could be improved or if you find something redundant. I just change lines on a whim because I'm too lazy to spend the time really learning the ins and outs of the UPDATE CC=clang
CFLAGS += -Xclang -fopenmp
LDFLAGS += -lomp I did have to download the libomp. Specifically, LLVM 14.0.6. Voilà! > install.packages('data.table', type = 'source')
> library(data.table)
data.table 1.14.8 using 5 threads (see ?getDTthreads). Latest news: r-datatable.com PS... |
@kevinushey could you please check if 1.13.2 resolves the problem for you? There was a change in #4735
|
Looks like this works for me as well. It sounds like the issue is ultimately resolved, then? Unless |
@kevinushey Great to hear it works.
That's an interesting idea. I don't have macOS and was waiting to see how macOS users got on with the latest version. I got the impression from https://mac.r-project.org/openmp/ that there are so many problems with macOS and OpenMP that there is not really much we can do. But, now that you said this, perhaps we could have another go. That would mean that the CRAN binary would be OpenMP-enabled again, iiuc? That would be great. If so, what exactly works for you: simply passing |
Right, and I can see arguments both ways:
I don't have a strong opinion on what the best way forward is, though. |
If data.table makes an attempt in the configure script, and performs the attempt correctly by falling back to no OpenMP, then it looks like we're agreeing on option 2 then. It's the performing-the-attempt-correctly part that is tricky cross-platform. Did you see the comment I put into |
I think you're right (sorry about that). It did ultimately work insofar that OpenMP was disabled by default on macOS, since the "default" OpenMP flags wouldn't work there. But it didn't help in turning OpenMP on. Regardless, I think getting this right would mean redoing that part of the |
@kevinushey I merged #4707 but now I fear that change is going to break it on macOS as it will go back to using the "default" OpenMP flags. Do you know the appropriate way to get the "-fopenmp" passed through? |
The main problem with the current approach, I think, is that To make things even more challenging, I think the comments in my original post are still correct; to enable OpenMP support, |
Thanks. Yes:
According to https://mac.r-project.org/openmp/, how about adding |
That looks good to me, especially since it has (some form of) R-Core's official blessing. |
Great. If I do the PR would you be able to test? (I don't have a Mac). |
Sure, I'd be happy to. |
@kevinushey any chance you're familiar enough with how the fix might go here to take up a PR here? It looks like it would benefit a lot of people. |
I do see some other packages on CRAN with Picking one from the hat: @jameslamb I see {lightgbm} looks to possibly be attempting OpenMP setup in its The PR introducing that part of |
👋🏻 I'm very familiar with that. It's been working well for me on my macbook and in LightGBM's continuous integration. And we haven't received user reports about OpenMP not being found, but that might just be because no one's noticed. It does seem like OpenMP is not being found successfully in our CRAN builds:
https://www.r-project.org/nosvn/R.check/r-release-macos-x86_64/lightgbm-00install.html So LightGBM's approach probably needs some tweaks to work with CRAN's setup. Some other discussions you might find useful:
You can |
That reminds me that it's also possible to submit a package tarball for testing at https://mac.r-project.org/macbuilder/submit.html; perhaps that could be used to further verify whether or not the PR in #6034 successfully detected OpenMP support on CRAN? |
Can anyone confirm this can be closed? Or do we need to wait for a CRAN release? |
I just uploaded it to macbuilder. See here for the results. According to this chunk of > test.data.table() # runs the main test suite of 5,000+ tests in /inst/tests/tests.Rraw
getDTthreads(verbose=TRUE):
OpenMP version (_OPENMP) 201811
omp_get_num_procs() 8
R_DATATABLE_NUM_PROCS_PERCENT unset (default 50)
R_DATATABLE_NUM_THREADS unset
R_DATATABLE_THROTTLE unset (default 1024)
omp_get_thread_limit() 2147483647
omp_get_max_threads() 8
OMP_THREAD_LIMIT unset
OMP_NUM_THREADS unset
RestoreAfterFork true
data.table is using 4 threads with throttle==1024. See ?setDTthreads.
test.data.table() running: /Volumes/PkgBuild/work/1720650797-9b56f65b10b20c0e/packages/big-sur-arm64/results/4.4/data.table.Rcheck/data.table/tests/tests.Rraw Thanks again @kevinushey |
With R 4.0.0, R (as well as packages on CRAN) will be built with the system toolchain (Apple Clang), which unfortunately mean OpenMP support is not provided natively by the compiler. Fortunately, it is still possible to use OpenMP with the system toolchain if you:
clang
to enable OpenMP with-Xpreprocessor -fopenmp
,libomp
during compilation,libomp
during link.Unfortunately,
data.table
fails to detect this case as it invokes the compiler with just-fopenmp
here:data.table/configure
Line 64 in b1b1832
It would be helpful for users on macOS if support for OpenMP on macOS could be detected + used.
Sorry, I recognize this is kind of a pain but figured it was worth filing since OpenMP is somewhat vital to data.table's performance.
See also:
http://r-sig-mac.29524.n8.nabble.com/R-SIG-Mac-Apple-Clang-does-support-OpenMP-if-libomp-is-available-td458.html#a461
(+ some other recent discussion on R-SIG-Mac)
The text was updated successfully, but these errors were encountered: