Skip to content

Conversation

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Dec 28, 2025

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 our src/ directory apart from passing the desired number of cores on.

We follow the same logic: if either option Ncpus is set (as is common via other R packages) or if OMP_THREAD_LIMIT is 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-rpkg org to try a second alternate r-universe build (given that my own has the pending Eigen 5.0.0 PR build)

@coatless
Copy link
Contributor

@eddelbuettel now you know why I have so so many organizations 😉

fi

## Now use all these
AC_SUBST([OPENMP_FLAG], ["${openmp_flag}"])
Copy link
Contributor

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?

Copy link
Member Author

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=""
fi

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

Copy link
Contributor

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.

Copy link

Copilot AI left a 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.

@eddelbuettel
Copy link
Member Author

@coatless No luck though. I don't think it ever built that branch of that added repo / entry in packages.json. Hmpf.

@eddelbuettel
Copy link
Member Author

I pivoted over at R universe and had this built as 0.4.9.8-0 and it is looking good. Screenshot to preserver it:

image

So I may merge this tomorrow, rebase and have the feature/eigen-5.0.0 branch be built there again as 0.4.9.9-0.

@eddelbuettel eddelbuettel merged commit 6108a3a into master Dec 29, 2025
4 checks passed
@eddelbuettel eddelbuettel deleted the feature/openmp branch December 29, 2025 13:08
This was referenced Dec 29, 2025
@eddelbuettel
Copy link
Member Author

One more follow-up mostly to keep @jaganmn in the loop: I learned from @aitap that the macOS is not all that polished so while we did all this work for all three OSs it may mostly work on the two not named macOS 🤷‍♂️ We win some, we loose some. At least we do have multicore processing on Linux.

@jaganmn
Copy link
Contributor

jaganmn commented Dec 30, 2025

AFAIK, CRAN distributions of R for macOS bundle libomp.dylib but not omp.h, and not all compilers include omp.h automatically with -fopenmp, so in general one must arrange for omp.h to be found on the preprocessor search path. If omp.h is not installed somewhere that R CMD config CPPFLAGS knows about, then the configure tests taken from data.table can fail. If you do not want to assume that the user is knowledgeable enough to have arranged for the preprocessor to find omp.h, then you would have to augment the configure logic, e.g., by testing for the existence of /usr/local/include/omp.h and then conditionally trying the configure test with -fopenmp -I/usr/local/include /usr/local/lib/libomp.dylib instead of -fopenmp -lomp.

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 config.log.

@eddelbuettel
Copy link
Member Author

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 omp.h. Now CRAN being CRAN we cannot fully look into Simon's setup so all we can do is poke with a long stick and hope he sorts it out. He usually does.

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?

@coatless
Copy link
Contributor

coatless commented Dec 30, 2025

I'm 100% in agreement that now is not the time to press for OpenMP on macOS. Maybe for 4.6.z sure.

@jaganmn
Copy link
Contributor

jaganmn commented Dec 30, 2025

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.

@eddelbuettel
Copy link
Member Author

eddelbuettel commented Dec 30, 2025

now is not the time to press for OpenMP on macOS.

How come? See how well the test package does on macOS arm64 at r-universe:

image

and ditto for amd64 at r-universe:

image

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 🤷‍♂️

@coatless
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make (Rcpp)Eigen use threads the way (Rcpp)Armadillo does

4 participants