-
-
Notifications
You must be signed in to change notification settings - Fork 219
Update OpenMP plugin to detect and handle Apple clang on macOS #1409
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
… between Apple clang and gcc compilers. Apple clang requires -Xclang -fopenmp flags and -lomp for linking, while gcc uses the standard -fopenmp flags. The plugin now: - Checks if running on macOS (Darwin) - Queries R CMD config CXX to get the compiler - Detects Apple clang by checking the compiler version string - Sets appropriate flags based on the detected compiler
eddelbuettel
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.
This looks good to me -- and Thank You!! for submitting it but I'd love to hear from @kevinushey too before merging.
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 updates the OpenMP plugin to properly detect and handle different compilers on macOS. Apple clang requires different flags than GCC for OpenMP support, and this change ensures the correct flags are used based on the detected compiler.
- Adds macOS detection and compiler identification logic
- Sets compiler-specific flags:
-Xclang -fopenmpand-lompfor Apple clang, standard-fopenmpfor GCC and other systems - Updates ChangeLog to document the changes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| R/Attributes.R | Enhanced the openmp plugin with macOS and Apple clang detection logic to set appropriate compiler flags |
| ChangeLog | Added entry documenting the OpenMP plugin update for macOS |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kevinushey
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.
LGTM but added some comments for supporting LLVM clang if appropriate.
|
|
||
| if (length(cxx) > 0 && nzchar(cxx[1])) { | ||
| # Extract just the compiler command (remove any flags) | ||
| cxx_compiler <- strsplit(cxx[1], "\\s+")[[1]][1] |
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.
Nit: some might use something like ccache clang <...> or other auxiliary tools when invoking the compiler. I think we should instead the compiler command plus flags via system().
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.
Agreed. I also default to having ccache on all the time and about one in a few hundred CRAN packages fails because it picks position one out of such a vector.
@coatless Any thoughts?
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.
Okay, I'll make that happen.
clang++ -arch arm64 -std=gnu++17 --version
# Apple clang version 17.0.0 (clang-1700.4.4.1)
# Target: arm64-apple-darwin25.1.0
# Thread model: posix
# InstalledDir: /Library/Developer/CommandLineTools/usr/bin| libs <- "-fopenmp" | ||
|
|
||
| # Check if we're on macOS | ||
| if (Sys.info()[['sysname']] == "Darwin") { |
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.
Pedantic but why not be consistent in use of quotation styles?
| cxxflags <- "-Xclang -fopenmp" | ||
| libs <- "-lomp" | ||
| } | ||
| # Otherwise it's gcc and regular -fopenmp works |
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.
Do we want to support Homebrew here? Or LLVM clang?
-I/opt/homebrew/opt/libomp/include
-L/opt/homebrew/opt/libomp/lib -lomp
Or /usr/local on Intel macOS machines.
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.
@coatless that is your ballpark
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.
We can add a detection of Homebrew; but, I'd like to skip macports/nix/pkgsrc/ others.
Though, I'm trying to remember if the R formula Is built with gcc instead of llvm.
Note
brew install r compiles R from scratch using brew's setup.
brew install --cask r installs the official CRAN R binary in a headless session.
This method may only target the later with flags.
$(brew --prefix llvm)/bin/clang --version
# Homebrew clang version 21.1.5
# Target: arm64-apple-darwin25.1.0
# Thread model: posix
# InstalledDir: /opt/homebrew/Cellar/llvm/21.1.5/bin
# Configuration file: /opt/homebrew/etc/clang/arm64-apple-darwin25.cfgDetection would be a similar grepl for Homebrew on the compiler_version call.
For the Intel vs. ARM macOS, there would need to be a grep on Target.
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.
You two are my expert macOS users, and @kevinushey clearly has enough exposure to the breadth of possible installations. So I rely on both of you: What should we do here?
Pull Request Template for Rcpp
Please explain the changes you want to apply to Rcpp, preferably in an issue ticket before you create a pull request. See the file Contributing and the other templates for details.
Checklist
R CMD checkstill passes all testsDetails
Modified the openmp plugin to properly detect macOS and differentiate
between Apple clang and gcc compilers. Apple clang requires
-Xclang -fopenmpflags and-lompfor linking, whilegccuses the standard
-fopenmpflags.The plugin now:
Discussion
For details, see: