-
Notifications
You must be signed in to change notification settings - Fork 56
Further macOS OpenMP tweaks #497
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
base: master
Are you sure you want to change the base?
Conversation
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 improves OpenMP support on macOS by removing the previous blanket special-casing and introducing a more flexible configuration approach that respects compiler capabilities while still allowing opt-out.
Key changes:
- Introduced macOS-specific OpenMP detection that enables OpenMP by default when the compiler supports it (
_OPENMPis defined), with an opt-out mechanism via a new configuration macro - Removed the blanket exclusion of OpenMP flags for macOS in the inline plugin function
- Conditionally restricted
ARMA_CRIPPLED_LAPACKdefinition to only legacy Armadillo versions on Windows
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| inst/include/RcppArmadillo/config/RcppArmadilloConfigGenerated.h.in | Adds macOS-specific OpenMP logic with conditional enabling based on compiler support and new opt-out macro; updates copyright years |
| inst/include/RcppArmadillo/config/RcppArmadilloConfig.h | Restricts ARMA_CRIPPLED_LAPACK to legacy Armadillo versions only; updates copyright years |
| R/inline.R | Removes macOS special case for OpenMP flags, now consistently uses $(SHLIB_OPENMP_CFLAGS) across all platforms; updates copyright year |
| ChangeLog | Documents the R/inline.R change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
inst/include/RcppArmadillo/config/RcppArmadilloConfigGenerated.h.in
Outdated
Show resolved
Hide resolved
coatless
left a comment
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.
In short, this is solid; however, we need to tweak Rcpp::plugins(openmp).
Specifics:
- We now get the appropriate number of cores if the local computer configuration has OpenMP.
- under the apple compiler setting we're still setting the
#define ARMA_DONT_USE_OPENMP 1.
- under the apple compiler setting we're still setting the
- Users must explicitly set in their
~/.R/Makevarsthe required OpenMP flags to opt-in:
CPPFLAGS += -Xclang -fopenmp
LDFLAGS += -lomp- If we switch over to trying to compile with OpenMP enabled via
// [[Rcpp::plugins(openmp)]]we're still getting a compile error on a locally configured mac due to only-fopenmpbeing set instead of-Xclang -fopenmp.
Failed compilation using `Rcpp::plugins(openmp)`
#include <RcppArmadillo.h>
// [[Rcpp::depends(RcppArmadillo)]]
// [[Rcpp::plugins(openmp)]]
// [[Rcpp::export]]
arma::ivec openmp_limits() {
arma::ivec limit_data(3);
#ifdef _OPENMP
limit_data[0] = omp_get_num_procs();
limit_data[1] = omp_get_max_threads();
limit_data[2] = omp_get_thread_limit();
#endif
return limit_data;
}
// [[Rcpp::export]]
bool openmp_enabled() {
bool enabled = false;
#ifdef _OPENMP
enabled = true;
#endif
return enabled;
}
// [[Rcpp::export]]
arma::ivec timesTwo(arma::ivec x) {
#pragma omp parallel for
for (size_t i = 0; i < x.size(); ++i) {
x[i] *= 2;
}
return x ;
}
/*** R
openmp_enabled()
openmp_limits()
timesTwo(42)
*/using C++ compiler: ‘Apple clang version 17.0.0 (clang-1700.4.4.1)’
using SDK: ‘MacOSX26.1.sdk’
clang++ -arch arm64 -std=gnu++17 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -I"/Library/Frameworks/R.framework/Versions/4.5-arm64/Resources/library/Rcpp/include" -I"/Library/Frameworks/R.framework/Versions/4.5-arm64/Resources/library/RcppArmadillo/include" -I"/Users/ronin/Documents/GitHub/rcppcore/RcppArmadillo" -I/opt/R/arm64/include -I/opt/R/arm64/include -fopenmp -fPIC -falign-functions=64 -Wall -g -O2 -c test.cpp -o test.o
clang++: error: unsupported option '-fopenmp'
make: *** [test.o] Error 1
Error in Rcpp::sourceCpp("test.cpp") :
Error 1 occurred building shared library.Standalone compilation via `sourceCpp()` with explicit `~/.R/Makevars` set
Set in ~/.R/Makevars:
CPPFLAGS += -Xclang -fopenmp
LDFLAGS += -lompSame script without plugins(openmp).
#include <RcppArmadillo.h>
// [[Rcpp::depends(RcppArmadillo)]]
// [[Rcpp::export]]
arma::ivec openmp_limits() {
arma::ivec limit_data(3);
#ifdef _OPENMP
limit_data[0] = omp_get_num_procs();
limit_data[1] = omp_get_max_threads();
limit_data[2] = omp_get_thread_limit();
#endif
return limit_data;
}
// [[Rcpp::export]]
bool openmp_enabled() {
bool enabled = false;
#ifdef _OPENMP
enabled = true;
#endif
return enabled;
}
// [[Rcpp::export]]
arma::ivec timesTwo(arma::ivec x) {
#pragma omp parallel for
for (size_t i = 0; i < x.size(); ++i) {
x[i] *= 2;
}
return x ;
}
/*** R
openmp_enabled()
openmp_limits()
timesTwo(42)
*/Compilation trail
/Library/Frameworks/R.framework/Resources/bin/R CMD SHLIB --preclean -o 'sourceCpp_2.so' 'test.cpp'
using C++ compiler: ‘Apple clang version 17.0.0 (clang-1700.4.4.1)’
using SDK: ‘MacOSX26.1.sdk’
clang++ -arch arm64 -std=gnu++17 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I../inst/include -I"/Library/Frameworks/R.framework/Versions/4.5-arm64/Resources/library/Rcpp/include" -I"/Library/Frameworks/R.framework/Versions/4.5-arm64/Resources/library/RcppArmadillo/include" -I"/Users/ronin/Documents/GitHub/rcppcore/RcppArmadillo" -I/opt/R/arm64/include -I/opt/R/arm64/include -Xclang -fopenmp -fPIC -falign-functions=64 -Wall -g -O2 -c test.cpp -o test.o
clang++ -arch arm64 -std=gnu++17 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -L/Library/Frameworks/R.framework/Resources/lib -L/opt/R/arm64/lib -L/opt/R/arm64/lib -lomp -o sourceCpp_2.so test.o -L/Library/Frameworks/R.framework/Resources/lib -lRlapack -L/Library/Frameworks/R.framework/Resources/lib -lRblas -L/opt/gfortran/lib/gcc/aarch64-apple-darwin20.0/14.2.0 -L/opt/gfortran/lib -lemutls_w -lheapt_w -lgfortran -lquadmath -F/Library/Frameworks/R.framework/.. -framework R
Output
> openmp_enabled()
[1] TRUE
> openmp_limits()
[,1]
[1,] 12
[2,] 12
[3,] 2147483647
> timesTwo(42)
[,1]
[1,] 84| inlineCxxPlugin <- function(...) { | ||
| ismacos <- Sys.info()[["sysname"]] == "Darwin" | ||
| openmpflag <- if (ismacos) "" else "$(SHLIB_OPENMP_CFLAGS)" | ||
| openmpflag <- "$(SHLIB_OPENMP_CFLAGS)" |
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.
Shouldn't this be SHLIB_OPENMP_CXXFLAGS instead?
| openmpflag <- "$(SHLIB_OPENMP_CFLAGS)" | |
| openmpflag <- "$(SHLIB_OPENMP_CXXFLAGS)" |
| #if !defined(ARMA_USE_OPENMP) | ||
| #if defined(__APPLE__) && defined(_OPENMP) | ||
| // User has OpenMP available, but check if they want it disabled | ||
| #ifndef RCPPARMA_MACOS_DISABLE_OPENMP |
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.
Is the standard prefix RCPPARMA? Or should we go RCPPARMADILLO ?
As discussed with @coatless in #493 -- marking it draft for now.
Edit: Draft status now removed.