-
Notifications
You must be signed in to change notification settings - Fork 42
Support OpenMP #145
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 #145
Conversation
|
@eddelbuettel now you know why I have so so many organizations 😉 |
| fi | ||
|
|
||
| ## Now use all these | ||
| AC_SUBST([OPENMP_FLAG], ["${openmp_flag}"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I understand, what purpose does OPENMP_FLAG serve given that you already accumulate in PKG_CXXFLAGS and PKG_LIBS options for linking libomp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I recall one (or two) of the three macOS variants needs it / sets it as relying on SHLIB_OPENMP_CXXFLAGS is not sufficient there and we need PKG_{CXXFLAGS,LIBS} there. However, in the simpler (Linux) case we do not catch these and set this one as a simpler fallback right before the use you quote:
# Overall summary
AC_MSG_CHECKING([for OpenMP])
if test x"${can_use_openmp}" = x"yes"; then
AC_MSG_RESULT([found and suitable])
openmp_flag='$(SHLIB_OPENMP_CXXFLAGS)'
else
AC_MSG_RESULT([missing so no OpenMP acceleration])
openmp_flag=""
fiThe pattern I borrow here from myself in RcppArmadillo has taken a few weeks to mature and now seems to work reliably so I thought it was beneficial to use it here too. The configure.ac is way more complicated that I would have wanted -- on Linux I simply place SHLIB_OPENMP_CXXFLAGS in there unconditionally and R takes care of it -- but macOS appears to require special love and care we now give it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking about accidental duplication of options in the concatenation of SHLIB_OPENMP_CXXFLAGS and PKG_CXXFLAGS, but I see now that it's precluded thanks to the initial "works out of the box" test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds OpenMP support to RcppEigen to enable multi-threaded parallel computations within the Eigen library. The implementation follows the approach from RcppArmadillo with robust support for Linux/Windows and enhanced macOS detection.
Key changes:
- Adds autoconf-based OpenMP detection with platform-specific fallbacks
- Implements thread control functions and automatic core throttling respecting R options and CRAN limits
- Updates documentation to describe threading behavior and control functions
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| configure.ac | Adds autoconf script for OpenMP detection across Linux, macOS, and Windows |
| configure | Generated configure script from autoconf |
| src/Makevars.in | Template for Makevars with OpenMP flag substitution |
| src/Makevars.win | Windows Makefile updated to use SHLIB_OPENMP_CXXFLAGS |
| src/Makevars | Removed static Makevars (now generated from .in template) |
| src/RcppEigen.cpp | Adds EigenSetNbThreads function and refactors eigen_version |
| src/RcppExports.cpp | Exports new thread control functions |
| R/init.R | New startup code to set initial thread count based on Ncpus and OMP_THREAD_LIMIT |
| R/RcppExports.R | Exports EigenSetNbThreads and eigen_version_typed |
| NAMESPACE | Exports new thread control functions |
| man/RcppEigen_throttle_cores.Rd | Documents thread control API |
| man/RcppEigen-package.Rd | Updates package documentation with threading information |
| cleanup | Adds config.log and config.status to cleanup targets |
| DESCRIPTION | Adds RoxygenNote field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coatless No luck though. I don't think it ever built that branch of that added repo / entry in packages.json. Hmpf. |
|
AFAIK, CRAN distributions of R for macOS bundle Well, I could be completely off the mark, but my guess is that configure test failures on some of the CRAN machines are related to the above. It would be nice to see a |
|
Hi @jaganmn -- it has also come to my attention that something at CRAN appears to be misconfigured. I had extensive emails about this with @aitap (who wrote a fine little test package for OpenMP detection on codeberg, I shadowed this over here to try GitHub Actions on it), and have been made aware of even more extensive discussions on r-sig-mac during December (see here) also involving a lot of input from @jeroen (and as I recall from you). It should also point out that for me the very package currently bonking at CRAN builds fine under both macOS variants on r-universe. I think a fair summary of all this is that CRAN is indeed borked right, that this may be temporary, and as you also point out, related to a missing Moreover, at the end of the day I am perfectly ready to adopt a 100% cynical view: even if it does not work for macOS, there is strictly no reason to deprive Linux and Windows users of OpenMP support now that we have it. I have spent a fair amount of time studying what e.g. data.table does but if it simply does not work at CRAN I can simply rip the configire{,.ac} files back out and sit back and rely on the fact that it works on Linux and Windows basically 'as is' with R taking care of it. That is what the economists call a 'Pareto-efficient improvement': nobody is worse off if we move from the status quo of 'number of threads equal to one' on all platforms to an situation where we manage to give 'full openmp support on two platforms'. It is a net gain for two out of three with no loss for the third. I am happy and willing to do some bits of work for macOS, but I do not see why failure of macOS to function as a normally usable developer environment should set other systems back. OpenMP support, even if Linux only, is a net gain for RcppEigen. And we appear to (these days) even get Windows thrown in. Any additional thoughts? |
|
I'm 100% in agreement that now is not the time to press for OpenMP on macOS. Maybe for 4.6.z sure. |
|
Yeah, I'm fine with where things are at here. My comment was partly for the benefit of Ivan in the off chance that he'd not figured everything out already. |
How come? See how well the test package does on macOS arm64 at r-universe:
and ditto for amd64 at r-universe:
Both (for now) accessible at https://github.com/r-universe/eddelbuettel/actions/runs/20584832288/job/59119420383 but likely easily re-producible. (This is from the package by @aitap; but it uses the exact same setup RcppArmadillo and RcppEigen try to rely on. I should make them show core count too via a short file in tests/.) I have no horse in this race but I fail to see why Simon could not get an email or two encouraging him to correct a shortcoming affecting many when the same stack works just fine elsewhere -- as per @jeroen this uses CRAN parts. But then again I am also aware how "challenging" it can be to get some parts in motion at CRAN so 🤷♂️ |
|
@eddelbuettel I like things going fast, I truly do. I love macOS despite better computer software being available for a nickel. But, at some point, the fight on placing OpenMP over on macOS is in a losing battle for the general audience especially with the current CRAN build. Back when this first started, I tried to I detail what was going on over at RcppCore/RcppArmadillo#493 (comment) and RcppCore/RcppArmadillo#493 (comment) . In short, having everything in place is good; but, we really need an updated baseline available in R 4.6.z. It's unlikely that Simon can do anything before that anyways. |



Closes #144
See the discussion in #144 and the motivating examples. This branch (and PR) follows RcppArmadillo in i) relying on the robust support under Linux and Windows where we can trust the
$(SHLIB_OPENMP_CXXFLAGS)variable and ii) fairly refined and extensive additional testing for the various ways in which macOS may or may not have OpenMP support which, which arguably overly complicated, has been seen to work there. Our use case is actually even a little simpler: All OpenMP uses happen inside of Eigen, none are in oursrc/directory apart from passing the desired number of cores on.We follow the same logic: if either option
Ncpusis set (as is common via other R packages) or ifOMP_THREAD_LIMITis set (as is the case for CRAN) then it is respected. Otherwise we let Eigen use what it sees, which may be all cores.This change should lead to a net speedup in the majority of use cases on all three operating systems.
/cc @coatless as I also just borrowed his
coatless-rpkgorg to try a second alternate r-universe build (given that my own has the pending Eigen 5.0.0 PR build)