also check plain '-fopenmp' for gcc#6418
Merged
MichaelChirico merged 4 commits intomasterfrom Aug 28, 2024
Merged
Conversation
MichaelChirico
changed the title
|
@kevinushey Thank you for addressing this. At least in my case it works with gcc. Here I only apply your patch from e0bee18 and it builds and links correctly: |
|
I also ran tests, everything passes: |
Member
|
Thanks @kevinushey and @barracuda156 for confirming. The change looks reasonable to me, let's add NEWS and ship it :) |
MichaelChirico
approved these changes
Aug 28, 2024
Anirban166
reviewed
Aug 29, 2024
| fi | ||
|
|
||
| # https://github.com/Rdatatable/data.table/issues/6409 | ||
| printf "%s" "* checking if R installation supports OpenMP with \"-fopenmp\" ... " |
Member
There was a problem hiding this comment.
LGTM!
To avoid the slight repetition with the check above though, you could consider merging them (if it still maintains readability), maybe something like:
printf "%s" "* Checking if R installation supports OpenMP with \"-Xclang -fopenmp\" or \"-fopenmp\" ... "
if CPPFLAGS="${CPPFLAGS} -Xclang -fopenmp" PKG_LIBS="-lomp" "${R_HOME}/bin/R" CMD SHLIB test-omp.c >> config.log 2>&1 || \
CPPFLAGS="${CPPFLAGS} -fopenmp" "${R_HOME}/bin/R" CMD SHLIB test-omp.c >> config.log 2>&1; then
echo "yes"
if ${CC} --version 2>&1 | grep -q "clang"; then
PKG_CFLAGS="${PKG_CFLAGS} -Xclang -fopenmp"
PKG_LIBS="${PKG_LIBS} -lomp"
else # This probably supports a few other GCC-compatible compilers too I reckon (such as ICC and PGI perhaps), so having a generic else sounds reasonable as opposed to being checked for GCC only.
PKG_CFLAGS="${PKG_CFLAGS} -fopenmp"
fi
export PKG_CFLAGS
export PKG_LIBS
export R_OPENMP_ENABLED=1
return
else
echo "no"
fiinstead of:
printf "%s" "* checking if R installation supports OpenMP with \"-Xclang -fopenmp\" ... "
if CPPFLAGS="${CPPFLAGS} -Xclang -fopenmp" PKG_LIBS="-lomp" "${R_HOME}/bin/R" CMD SHLIB test-omp.c >> config.log 2>&1; then
echo "yes"
export PKG_CFLAGS="${PKG_CFLAGS} -Xclang -fopenmp"
export PKG_LIBS="${PKG_LIBS} -lomp"
export R_OPENMP_ENABLED=1
return
else
echo "no"
fi
printf "%s" "* checking if R installation supports OpenMP with \"-fopenmp\" ... "
if CPPFLAGS="${CPPFLAGS} -fopenmp" "${R_HOME}/bin/R" CMD SHLIB test-omp.c >> config.log 2>&1; then
echo "yes"
export PKG_CFLAGS="${PKG_CFLAGS} -fopenmp"
export R_OPENMP_ENABLED=1
return
else
echo "no"
fi
Member
There was a problem hiding this comment.
Also: Unrelated to this PR, but a minor nit while we're discussing this file - 'openmp' > 'OpenMP' here: (for consistency with the other messages)
Line 99 in 392a321
MichaelChirico
added a commit
that referenced
this pull request
Aug 30, 2024
* also check plain '-fopenmp' for gcc (closes #6409) * update NEWS * cite in NEWS --------- Co-authored-by: Michael Chirico <chiricom@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6409
@barracuda156 is this sufficient?