Skip to content

Detect OpenMP support#3984

Merged
mattdowle merged 5 commits intoRdatatable:masterfrom
jimhester:detect-openmp
Dec 20, 2019
Merged

Detect OpenMP support#3984
mattdowle merged 5 commits intoRdatatable:masterfrom
jimhester:detect-openmp

Conversation

@jimhester
Copy link
Copy Markdown
Contributor

This detects support for OpenMP and only includes the compiler flags for
it if OpenMP is supported.

This solves the common case on macOS, because R on macOS ships with
-fopenmp in SHLIB_OPENMP_CFLAGS, as the CRAN compiler toolchain does
support OpenMP, however the XCode toolchain distributed by Apple does
not
support OpenMP, which causes data.table to fail to compile from
source.

Fixes #2161

This detects support for OpenMP and only includes the compiler flags for
it if OpenMP is supported.

This solves the common case on macOS, because R on macOS ships with
`-fopenmp` in SHLIB_OPENMP_CFLAGS, as the CRAN compiler toolchain _does_
support OpenMP, however the XCode toolchain distributed by Apple _does
 not_ support OpenMP, which causes data.table to fail to compile from
 source.

Fixes Rdatatable#2161
Comment thread .Rbuildignore
^NEWS\.0\.md$
^README\.md$
^_pkgdown\.yml$
^src/Makevars$
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need to ignore the generated Makevars in both git and the built package, we generate it from src/Makevars.in in the configure script.

Comment thread cleanup
@@ -0,0 +1,2 @@
#!/bin/sh
rm -f src/Makevars
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need to cleanup the generated file with the cleanup script

Comment thread configure
CPPFLAGS=`${R_HOME}/bin/R CMD config CPPFLAGS`

# Test if we have a OPENMP compatible compiler
echo "#include <omp.h>\nint main () { return omp_get_num_threads (); }" | ${CC} ${CPPFLAGS} ${PKG_CFLAGS} ${CFLAGS} ${SHLIB_OPENMP_CFLAGS} -E -xc - >/dev/null 2>&1 || R_NO_OPENMP=1;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here we test a simple OpenMP program with what is in SHLIB_OPENMP_CFLAGS to verify that our current compiler supports OpenMP.

Comment thread src/Makevars.win
@@ -0,0 +1,5 @@
PKG_CFLAGS = $(SHLIB_OPENMP_CFLAGS)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

configure will only run on non-Windows, which means that src/Makevars will not be generated there, so we need to supply Makevars.win for windows. But we don't really need to check if OpenMP is supported, because the Rtools compilers all support it.

Comment thread configure
Comment on lines +64 to +67
sed -e "s|@openmp_cflags@||" src/Makevars.in > src/Makevars
else
echo "OpenMP supported"
sed -e "s|@openmp_cflags@|\$(SHLIB_OPENMP_CFLAGS)|" src/Makevars.in > src/Makevars
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Then we replace the placeholder @openmp_cflags@ with either nothing if the compiler doesn't support it or SHLIB_OPENMP_CFLAGS if it does.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 21, 2019

Codecov Report

Merging #3984 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3984   +/-   ##
======================================
  Coverage    99.4%   99.4%           
======================================
  Files          72      72           
  Lines       13675   13675           
======================================
  Hits        13594   13594           
  Misses         81      81

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eba8704...588b709. Read the comment docs.

Comment thread configure
@jimhester
Copy link
Copy Markdown
Contributor Author

@MichaelChirico I added a message linking to the wiki in 576c852, let me know if there is anything else I can do.

@jangorecki jangorecki added this to the 1.12.7 milestone Dec 6, 2019
@mattdowle mattdowle modified the milestones: 1.12.7, 1.12.9 Dec 8, 2019
@mattdowle
Copy link
Copy Markdown
Member

Thanks, @jimhester! This looks great. The message is great to see on installation. There is a package startup message too if it detects it wasn't compiled at that point.
I've invited you to be project member. You'll need to accept the invite that shows in your GitHub profile or GitHub repository which I'm sure you know but for completeness just in case. I'll move the news item up and add you in the contributors list in a follow up commit to master. (I could commit to the fork but since the PR is passing and ready and green to merge, it's faster for me this way.)

@mattdowle mattdowle merged commit e6c9c8d into Rdatatable:master Dec 20, 2019
mattdowle added a commit that referenced this pull request Dec 20, 2019
mattdowle added a commit that referenced this pull request Dec 20, 2019
…t even worked before or what changed. #3984 just merged seems likely, but I don't see why. It's more like cc.R was already wrong and its luck ran out. Oh well, moving on. cc() works again now and I checked R CMD INSTALL already still created datatable.so without dot as before.
@jangorecki jangorecki modified the milestones: 1.12.11, 1.12.9 May 26, 2020
@mattdowle mattdowle mentioned this pull request Aug 4, 2020
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.

[Request] Test for compiler compatibility with -fopenmp before using SHLIB_OPENMP_CFLAGS

5 participants