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

OpenMP-detection on macOS systems #219

Merged
merged 2 commits into from Apr 20, 2018

Conversation

Projects
None yet
4 participants
@kthohr
Contributor

kthohr commented Apr 20, 2018

Better detection of OpenMP-compatible compilers on macOS systems.

Ref: /pull/170

@kthohr kthohr changed the title from update configure script to OpenMP-detection on macOS systems Apr 20, 2018

@coatless

This comment has been minimized.

Contributor

coatless commented Apr 20, 2018

Quick notes regarding the approach I mentioned in #170 :

  1. This does not test for the presence of OpenMP (it's a bit anti-autoconf in nature) but instead tests against compiler version.
  2. Just like the prior approach, we do not reconcile the ability of the clang4 binary provided by CRAN to enable OpenMP. (e.g. This fails to detect it.)

I'd feel a bit more comfortable incorporating them4 routine R uses c.f.

https://github.com/wch/r-source/blob/a71d8f00c109a339b40f9ac42d2e9116d986279f/m4/openmp.m4#L34-L100

Results can be found here:

coatless/RcppArmadillo@3f1aed6

devtools::install_github("coatless/rcpparmadillo", ref=" feature/macos-openmp-detection")

I'd love to get Simon's (@s-u) take on why the m4 routine is off related to detecting the clang4 OpenMP ability. For another set of eyes let's also ping @kevinushey.

@kthohr

This comment has been minimized.

Contributor

kthohr commented Apr 20, 2018

Thanks for your comments.

  1. My intention was to detect an OpenMP-compatible clang by version number, similar to the existing gcc check. In the case that ARMA_USE_OPENMP is enabled, but _OPENMP is missing (at compile time), then Armadillo should cover the break, unless I am missing something.

https://github.com/conradsnicta/armadillo-code/blob/unstable/include/armadillo_bits/compiler_setup.hpp#L460

  1. I made a minor change to cover the clang4 binary provided by CRAN. Can you check again?
@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Apr 20, 2018

For completeness, I should mention that RcppArmadillo does not use m4 / full autoconf on Linux either. So far we got by 'trusting' R's detection and Armadillo's tests. So I'm not uncomfortable with this approach here either.

So I thinnk we can merge this given the earlier (and largely successful) tests.

Comments?

@conradsnicta

This comment has been minimized.

Contributor

conradsnicta commented Apr 20, 2018

Aren't the apple-clang version numbers (as shipped with Xcode) deliberately different to upstream clang? If I recall correctly, Homebrew and/or Macports have a separate clang package available, which uses proper clang.

Apple uses a "marketing" version number, as reportedly a clueless corporate marketdroid thought that having a version number higher than gcc was the most defining feature of a compiler. Upstream clang never bought into this silliness.

This means that apple-clang version X does not correspond to upstream clang version X, so detecting features based on the version number is not a good idea. Instead, clang recommends to use feature availability macros, along these lines:
https://github.com/conradsnicta/armadillo-code/blob/5a0307714d154268412b1b3b3040dcb55cb330be/include/armadillo_bits/compiler_setup.hpp#L249

@kthohr

This comment has been minimized.

Contributor

kthohr commented Apr 20, 2018

@conradsnicta - You are correct about the version number. However, the line

apple_compiler=$($CXX --version 2>&1 | grep -i -c -e 'apple llvm')

detects Apple's version and disables OpenMP features. (If this test passes, then we look for a version number.)

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Apr 20, 2018

@conradsnicta Further, as the R build system is somewhat tight to guarantee success, there tends to be just one version on some OSs (exactly one gcc via R Core / CRAN on windoze; on macOS it is AFAICT one dominating version also from CRAN). But on macOS people can of course shoot themselves in the foot more easily by going via brew (and issue binary API deltas) just as people are wont to do by mixing, say, the anaconda stach with R's which generally results in tears.

For user / developers "who know" we put specific instruction into the FAQ and @coatless cooked up an entire installer script fetching the right version and setting the right hooks.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Apr 20, 2018

@conradsnicta And sorry, but I have to get this off my chest once every year: I find it extremely useful that you keep such a close look on what we do here with RcppArmadillo, that you are so responsive to out issues, and that you reach out to us for testing, but I am still shocked and disappointed that you continue to refuse to mention RcppArmadillo in the Armadillo documentation / links to related software. I asked you at least three or four times, you ignored me each time. I don't get it. As of today 468 packages on CRAN use RcppArmadillo but an unknown higher number of github and whatnot projects. Is that not something to share with some pride?

@coatless

This comment has been minimized.

Contributor

coatless commented Apr 20, 2018

@kthohr for the record, the reason I'm pushing for the OpenMP test to verify the compiler capabilities vs. versioning is because CRAN hosts a crippled version of clang (e.g. no openmp/ libomp-dev).

We ran into this issue circa August 2017. This caused us to move away from the SHLIB_OPENMP_* flag setup to customly setting the OPENMP_FLAGS.

I'd really like to know why the clang4 binary that resulted in the entire toolchain changing isn't been detected as OpenMP-compliant and, thus, providing the benefits of the change.

Also, Brian's clang check fails with

In file included from sparse.cpp:23:
In file included from /data/gannet/ripley/R/packages/tests-clang/RcppArmadillo.Rcheck/RcppArmadillo/include/RcppArmadillo.h:31:
In file included from /data/gannet/ripley/R/packages/tests-clang/RcppArmadillo.Rcheck/RcppArmadillo/include/RcppArmadilloForward.h:46:
/data/gannet/ripley/R/packages/tests-clang/RcppArmadillo.Rcheck/RcppArmadillo/include/armadillo:88:12: fatal error: 'omp.h' file not found
#include <omp.h>
^
1 warning and 1 error generated.

This actually works on my Clang flavor, because I have omp.h from having
installed Debian's libomp-dev. But ideally this test would be
conditional on OMP support?

@conradsnicta

This comment has been minimized.

Contributor

conradsnicta commented Apr 20, 2018

@eddelbuettel - Nothing related to pride; the arma download page simply aims for providing a deliberate shortlist of directly useful related stuff in pure C++ land, in order to enhance the surrounding ecosystem (this list is short on the download page in order to not overwhelm people with info). This is not necessary with RcppArmadillo, as it is already in a well established ecosystem within R land (with thanks to your hard work).

However, RcppArmadillo is explicitly listed in the Related Software section on Armadillo's faq page which is a longer list of software. In fact, RcppArmadillo is linked twice (or three times if you count our nice paper) on arma's faq page -- see also the "Is it possible to use Armadillo from other languages?" question on the Features section. I think that's plenty.

If you really want a link to RcppArmadillo on arma's download page, I can put it up, but I just don't see it serving a practical purpose. Someone wanting RcppArmadillo is not going to obtain it from arma's download page anyway -- they will use CRAN and the install_packages() command in R. In fact, having RcppArmadillo listed on the download page can confuse a good chunk of potential users, who may promptly install Armadillo in its pure C++ form (ie standalone library) and then wonder why it's not working in R. I've learned not to underestimate the silly things that people will do.

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Apr 20, 2018

@conradsnicta Fair points yet I also think RcppArmadillo drives a fair number of visitors to your site, and it would help you too if it had a simple 'for use of Armadillo from the R language see ...' (or alike) link. Anyway, I will berate you more over a beer once I get to Brisbane in July....

@eddelbuettel

This comment has been minimized.

Member

eddelbuettel commented Apr 20, 2018

So given the thumbs-up reaction in #170, I am merging this.

@eddelbuettel eddelbuettel merged commit 35216c2 into RcppCore:master Apr 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kthohr kthohr deleted the kthohr:macOS-openMP branch Apr 20, 2018

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