Skip to content

Conversation

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Nov 13, 2025

As discussed with @coatless in #493 -- marking it draft for now.

Edit: Draft status now removed.

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 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 (_OPENMP is 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_LAPACK definition 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.

@eddelbuettel eddelbuettel marked this pull request as ready for review November 15, 2025 00:02
Copy link
Contributor

@coatless coatless left a 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:

  1. 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.
  2. Users must explicitly set in their ~/.R/Makevars the required OpenMP flags to opt-in:
CPPFLAGS += -Xclang -fopenmp
LDFLAGS += -lomp
  1. 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 -fopenmp being 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 += -lomp

Same 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)"
Copy link
Contributor

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?

Suggested change
openmpflag <- "$(SHLIB_OPENMP_CFLAGS)"
openmpflag <- "$(SHLIB_OPENMP_CXXFLAGS)"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not matter in practice. There are three, they are always identical (?) and CRAN only complains if you use different ones between compiler and linker. I have long used the C variant for simplicity (though also probably not always).

edd@paul:~$ grep SHLIB_OPENMP_ /etc/R/Makeconf 
SHLIB_OPENMP_CFLAGS = -fopenmp
SHLIB_OPENMP_CXXFLAGS = -fopenmp
SHLIB_OPENMP_FFLAGS = -fopenmp
edd@paul:~$ 

#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
Copy link
Contributor

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 ?

Copy link
Member Author

@eddelbuettel eddelbuettel Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Both better and more consistent. Will switch to long form.

(I think I followed your suggestion letter by letter though 😜 )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eddelbuettel I know. I couldn't remember what was settled on for the prefix though post legacy/current defines.

@eddelbuettel
Copy link
Member Author

eddelbuettel commented Nov 15, 2025

I think given how messy the macOS situation is in general when it comes to OpenMP I am ok with requiring the file ~/.R/Makevars to have the right choices. Didn't using that file be a requirement anyway in the past in order to get the right compiler (flags) selected?

Or do you think we can do substantially better? By altering the openmp plugin in Rcpp?

Even if so (and we can look at that for Rcpp) it seems like this PR is ready and improves over the previous one? Big thank you for all the very detailed and helpful feedback.

@coatless
Copy link
Contributor

@eddelbuettel yes to a large extent. The other option is setting session options via PKG_CXXFLAGS / PKG_LIBS.

There shouldn't be anymore hold up on this PR.

I'll shoot over a change PR for Rcpp openmp plugin. There's four parts:

  1. Checks if macOS
  2. Pulls R CMD config CXX
  3. Queries compiler and checks if Apple clang
    • Enables -Xclang -fopenmp
    • Otherwise, it's gcc so the usual -fopenmp works.

I'm not going to add a check on the plugin itself for whether OpenMP is correctly setup.

The goal of making the plugins PR is to have some level of "unity" across standalone C++ scripts so there isn't a "gotcha" if // [[Rcpp::plugins(openmp)]] is declared but the end user runs into an issue because -fopenmp isn't setup for Clang.

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.

3 participants