From d624b6ef0882e32e5afd816a929d5af2f114b57e Mon Sep 17 00:00:00 2001 From: Ivan K Date: Thu, 5 Dec 2024 15:50:29 +0300 Subject: [PATCH 1/5] Rewrite OpenMP tests in configure * Test loading the resulting shared objects; make sure they return an above-zero number of threads * Test -fopenmp on all Unix-like operating systems --- configure | 79 +++++++++++++++----------------------- src/Makevars.in | 2 +- tools/check-openmp-flags.R | 39 +++++++++++++++++++ 3 files changed, 71 insertions(+), 49 deletions(-) create mode 100644 tools/check-openmp-flags.R diff --git a/configure b/configure index 2402ecef1..413840073 100755 --- a/configure +++ b/configure @@ -75,57 +75,42 @@ fi # necessarily here in configure). Hence use -fopenmp directly for this detection step. # printf not echo to pass checkbashisms w.r.t. to the \n -cat < test-omp.c -#include -int main() { - return omp_get_num_threads(); +check_openmp_flags () { + "${R_HOME}/bin/Rscript" tools/check-openmp-flags.R "$1" "$2" } -EOF detect_openmp () { + printf "%s" "* checking if R installation is configured to use OpenMP... " + if check_openmp_flags '$(SHLIB_OPENMP_CFLAGS)' '$(SHLIB_OPENMP_CFLAGS)' >> config.log 2>&1; then + echo "yes" + export R_OPENMP_CFLAGS='$(SHLIB_OPENMP_CFLAGS)' + export R_OPENMP_LIBS='$(SHLIB_OPENMP_CFLAGS)' + export R_OPENMP_ENABLED=1 + return + else + echo "no" + fi - if [ "$(uname)" = "Linux" ]; then - - printf "%s" "* checking if R installation supports OpenMP without any extra hints... " - if "${R_HOME}/bin/R" CMD SHLIB test-omp.c >> config.log 2>&1; then - echo "yes" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi - - - printf "%s" "* checking if R installation supports openmp with \"-fopenmp\" flag... " - if ${CC} ${CFLAGS} -fopenmp 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 - fi # uname=Linux + # https://github.com/Rdatatable/data.table/issues/6409 + printf "%s" "* checking if R installation supports openmp with \"-fopenmp\" flag... " + if check_openmp_flags -fopenmp -fopenmp >> config.log 2>&1; then + echo "yes" + export R_OPENMP_CFLAGS="-fopenmp" + export R_OPENMP_LIBS="-fopenmp" + export R_OPENMP_ENABLED=1 + return + else + echo "no" + fi if [ "$(uname)" = "Darwin" ]; then # https://mac.r-project.org/openmp 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 - - # https://github.com/Rdatatable/data.table/issues/6409 - 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 + if check_openmp_flags "-Xclang -fopenmp" -lomp >> config.log 2>&1; then echo "yes" - export PKG_CFLAGS="${PKG_CFLAGS} -fopenmp" + export R_OPENMP_CFLAGS="-Xclang -fopenmp" + export R_OPENMP_LIBS="-lomp" export R_OPENMP_ENABLED=1 return else @@ -142,10 +127,10 @@ detect_openmp () { printf "%s" "* checking if libomp installation at ${HOMEBREW_PREFIX}/opt/libomp can be used... " LIBOMP_INCLUDE="-I${HOMEBREW_PREFIX}/opt/libomp/include -Xclang -fopenmp" LIBOMP_LINK="-L${HOMEBREW_PREFIX}/opt/libomp/lib -lomp" - if ${CC} ${CFLAGS} ${LIBOMP_INCLUDE} ${LIBOMP_LINK} test-omp.c >> config.log 2>&1; then + if check_openmp_flags "${LIBOMP_INCLUDE}" "${LIBOMP_LINK}" >> config.log 2>&1; then echo "yes" - export PKG_CFLAGS="${PKG_CFLAGS} ${LIBOMP_INCLUDE}" - export PKG_LIBS="${PKG_LIBS} ${LIBOMP_LINK}" + export R_OPENMP_CFLAGS="${LIBOMP_INCLUDE}" + export R_OPENMP_LIBS="${LIBOMP_LINK}" export R_OPENMP_ENABLED=1 return else @@ -160,8 +145,6 @@ detect_openmp () { } detect_openmp -# Clean up. -rm -f test-omp.* a.out if [ "${R_OPENMP_ENABLED}" = "0" ]; then echo "***" @@ -171,9 +154,9 @@ if [ "${R_OPENMP_ENABLED}" = "0" ]; then echo "*** https://github.com/Rdatatable/data.table/wiki/Installation" echo "*** Continuing installation without OpenMP support..." echo "***" - sed -e "s|@openmp_cflags@||" src/Makevars.in > src/Makevars + sed -e "s|@openmp_cflags@||" -e "s|@openmp_ldflags@||" src/Makevars.in > src/Makevars else - sed -e "s|@openmp_cflags@|\$(SHLIB_OPENMP_CFLAGS)|" src/Makevars.in > src/Makevars + sed -e "s|@openmp_cflags@|${R_OPENMP_CFLAGS}|" -e "s|@openmp_ldflags@|${R_OPENMP_LIBS}|" src/Makevars.in > src/Makevars fi # retain user supplied PKG_ env variables, #4664. See comments in Makevars.in too. diff --git a/src/Makevars.in b/src/Makevars.in index fcfaceba9..67fbb841f 100644 --- a/src/Makevars.in +++ b/src/Makevars.in @@ -1,5 +1,5 @@ PKG_CFLAGS = @PKG_CFLAGS@ @openmp_cflags@ @zlib_cflags@ -PKG_LIBS = @PKG_LIBS@ @openmp_cflags@ @zlib_libs@ +PKG_LIBS = @PKG_LIBS@ @openmp_ldflags@ @zlib_libs@ # See WRE $1.2.1.1. But retain user supplied PKG_* too, #4664. # WRE states ($1.6) that += isn't portable and that we aren't allowed to use it. # Otherwise we could use the much simpler PKG_LIBS += @openmp_cflags@ -lz. diff --git a/tools/check-openmp-flags.R b/tools/check-openmp-flags.R new file mode 100644 index 000000000..b48b439f3 --- /dev/null +++ b/tools/check-openmp-flags.R @@ -0,0 +1,39 @@ +args <- commandArgs(TRUE) +stopifnot(`Usage: Rscript check-openmp-flags.R CFLAGS LDFLAGS` = length(args) == 2) +# We'll need to create Makevars (and object and DLL files too) +setwd(tempdir()) +cat(sprintf( + "Testing if OpenMP works with CFLAGS='%s' and LDFLAGS='%s':\n", + args[[1]], args[[2]] +)) + +# It must be a 'Makevars' for constructs like $(SHLIB_OPENMP_CFLAGS) to work: +writeLines(c( + paste("PKG_CFLAGS =", args[[1]]), + paste("PKG_LIBS =", args[[2]]) +), "Makevars") + +# Will succeed to compile even without OpenMP but return 0 +writeLines(" +#ifdef _OPENMP + #include +#endif +void test_openmp(int * numprocs) { + *numprocs = +#ifdef _OPENMP + omp_get_max_threads() +#else + 0 +#endif + ; +} +", "test.c") + +# May fail to compile anyway +stopifnot(tools::Rcmd("SHLIB --preclean test.c") == 0) + +dyn.load(paste0("test", .Platform$dynlib.ext)) +success <- .C("test_openmp", ans = integer(1))$ans > 0 + +cat(sprintf("Result: %s\n", if (success) "yes" else "no")) +q("no", status = !success) From cf2bb48993d9591b8a5f709c0fc28fab6f049af7 Mon Sep 17 00:00:00 2001 From: Ivan K Date: Fri, 6 Dec 2024 12:26:51 +0300 Subject: [PATCH 2/5] Deduplicate most of the OpenMP checks * Make sure to include user-supplied PKG_{CFLAGS,LIBS} * Be consistent about calling the linker flags LIBS, not LDFLAGS --- configure | 57 +++++++++++--------------------------- src/Makevars.in | 2 +- tools/check-openmp-flags.R | 4 +-- 3 files changed, 19 insertions(+), 44 deletions(-) diff --git a/configure b/configure index 413840073..999db7aa1 100755 --- a/configure +++ b/configure @@ -75,47 +75,31 @@ fi # necessarily here in configure). Hence use -fopenmp directly for this detection step. # printf not echo to pass checkbashisms w.r.t. to the \n +# Usage: "user-facing string" "CFLAGS" "LDFLAGS" check_openmp_flags () { - "${R_HOME}/bin/Rscript" tools/check-openmp-flags.R "$1" "$2" -} - -detect_openmp () { - printf "%s" "* checking if R installation is configured to use OpenMP... " - if check_openmp_flags '$(SHLIB_OPENMP_CFLAGS)' '$(SHLIB_OPENMP_CFLAGS)' >> config.log 2>&1; then + printf "* checking if %s... " "$1" + if "${R_HOME}/bin/Rscript" tools/check-openmp-flags.R "${PKG_CFLAGS} $2" "${PKG_LIBS} $3" >> config.log 2>&1; then echo "yes" - export R_OPENMP_CFLAGS='$(SHLIB_OPENMP_CFLAGS)' - export R_OPENMP_LIBS='$(SHLIB_OPENMP_CFLAGS)' + export R_OPENMP_CFLAGS="$2" + export R_OPENMP_LIBS="$3" export R_OPENMP_ENABLED=1 - return + return 0 else echo "no" + return 1 fi +} + +detect_openmp () { + check_openmp_flags "R installation is configured to use OpenMP" '$(SHLIB_OPENMP_CFLAGS)' '$(SHLIB_OPENMP_CFLAGS)' && return # https://github.com/Rdatatable/data.table/issues/6409 - printf "%s" "* checking if R installation supports openmp with \"-fopenmp\" flag... " - if check_openmp_flags -fopenmp -fopenmp >> config.log 2>&1; then - echo "yes" - export R_OPENMP_CFLAGS="-fopenmp" - export R_OPENMP_LIBS="-fopenmp" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi + check_openmp_flags "R installation supports openmp with \"-fopenmp\" flag" -fopenmp -fopenmp && return if [ "$(uname)" = "Darwin" ]; then # https://mac.r-project.org/openmp - printf "%s" "* checking if R installation supports OpenMP with \"-Xclang -fopenmp\" ... " - if check_openmp_flags "-Xclang -fopenmp" -lomp >> config.log 2>&1; then - echo "yes" - export R_OPENMP_CFLAGS="-Xclang -fopenmp" - export R_OPENMP_LIBS="-lomp" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi + check_openmp_flags "R installation supports OpenMP with \"-Xclang -fopenmp\" " "-Xclang -fopenmp" "-lomp" && return if [ "$(uname -m)" = "arm64" ]; then HOMEBREW_PREFIX=/opt/homebrew @@ -124,18 +108,9 @@ detect_openmp () { fi if [ -e "${HOMEBREW_PREFIX}/opt/libomp" ]; then - printf "%s" "* checking if libomp installation at ${HOMEBREW_PREFIX}/opt/libomp can be used... " LIBOMP_INCLUDE="-I${HOMEBREW_PREFIX}/opt/libomp/include -Xclang -fopenmp" LIBOMP_LINK="-L${HOMEBREW_PREFIX}/opt/libomp/lib -lomp" - if check_openmp_flags "${LIBOMP_INCLUDE}" "${LIBOMP_LINK}" >> config.log 2>&1; then - echo "yes" - export R_OPENMP_CFLAGS="${LIBOMP_INCLUDE}" - export R_OPENMP_LIBS="${LIBOMP_LINK}" - export R_OPENMP_ENABLED=1 - return - else - echo "no" - fi + check_openmp_flags "libomp installation at ${HOMEBREW_PREFIX}/opt/libomp can be used" "${LIBOMP_INCLUDE}" "${LIBOMP_LINK}" && return fi fi # uname=Darwin @@ -154,9 +129,9 @@ if [ "${R_OPENMP_ENABLED}" = "0" ]; then echo "*** https://github.com/Rdatatable/data.table/wiki/Installation" echo "*** Continuing installation without OpenMP support..." echo "***" - sed -e "s|@openmp_cflags@||" -e "s|@openmp_ldflags@||" src/Makevars.in > src/Makevars + sed -e "s|@openmp_cflags@||" -e "s|@openmp_libs@||" src/Makevars.in > src/Makevars else - sed -e "s|@openmp_cflags@|${R_OPENMP_CFLAGS}|" -e "s|@openmp_ldflags@|${R_OPENMP_LIBS}|" src/Makevars.in > src/Makevars + sed -e "s|@openmp_cflags@|${R_OPENMP_CFLAGS}|" -e "s|@openmp_libs@|${R_OPENMP_LIBS}|" src/Makevars.in > src/Makevars fi # retain user supplied PKG_ env variables, #4664. See comments in Makevars.in too. diff --git a/src/Makevars.in b/src/Makevars.in index 67fbb841f..d98c69ca2 100644 --- a/src/Makevars.in +++ b/src/Makevars.in @@ -1,5 +1,5 @@ PKG_CFLAGS = @PKG_CFLAGS@ @openmp_cflags@ @zlib_cflags@ -PKG_LIBS = @PKG_LIBS@ @openmp_ldflags@ @zlib_libs@ +PKG_LIBS = @PKG_LIBS@ @openmp_libs@ @zlib_libs@ # See WRE $1.2.1.1. But retain user supplied PKG_* too, #4664. # WRE states ($1.6) that += isn't portable and that we aren't allowed to use it. # Otherwise we could use the much simpler PKG_LIBS += @openmp_cflags@ -lz. diff --git a/tools/check-openmp-flags.R b/tools/check-openmp-flags.R index b48b439f3..4131df732 100644 --- a/tools/check-openmp-flags.R +++ b/tools/check-openmp-flags.R @@ -1,9 +1,9 @@ args <- commandArgs(TRUE) -stopifnot(`Usage: Rscript check-openmp-flags.R CFLAGS LDFLAGS` = length(args) == 2) +stopifnot(`Usage: Rscript check-openmp-flags.R CFLAGS LIBS` = length(args) == 2) # We'll need to create Makevars (and object and DLL files too) setwd(tempdir()) cat(sprintf( - "Testing if OpenMP works with CFLAGS='%s' and LDFLAGS='%s':\n", + "Testing if OpenMP works with CFLAGS='%s' and LIBS='%s':\n", args[[1]], args[[2]] )) From 70cb593412f9665a09289b110c97751a78bc9c51 Mon Sep 17 00:00:00 2001 From: Ivan K Date: Fri, 6 Dec 2024 12:47:22 +0300 Subject: [PATCH 3/5] Capitalise OpenMP properly --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure b/configure index 999db7aa1..11a0aa947 100755 --- a/configure +++ b/configure @@ -94,7 +94,7 @@ detect_openmp () { check_openmp_flags "R installation is configured to use OpenMP" '$(SHLIB_OPENMP_CFLAGS)' '$(SHLIB_OPENMP_CFLAGS)' && return # https://github.com/Rdatatable/data.table/issues/6409 - check_openmp_flags "R installation supports openmp with \"-fopenmp\" flag" -fopenmp -fopenmp && return + check_openmp_flags "R installation supports OpenMP with \"-fopenmp\" flag" -fopenmp -fopenmp && return if [ "$(uname)" = "Darwin" ]; then From cae9be60369d9cfcfa46ae9932d0d6351659b868 Mon Sep 17 00:00:00 2001 From: Ivan K Date: Mon, 9 Dec 2024 10:41:32 +0300 Subject: [PATCH 4/5] Deduplicate OpenMP substitution in Makevars --- configure | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/configure b/configure index 11a0aa947..14c92a350 100755 --- a/configure +++ b/configure @@ -91,6 +91,9 @@ check_openmp_flags () { } detect_openmp () { + R_OPENMP_CFLAGS= + R_OPENMP_LIBS= + check_openmp_flags "R installation is configured to use OpenMP" '$(SHLIB_OPENMP_CFLAGS)' '$(SHLIB_OPENMP_CFLAGS)' && return # https://github.com/Rdatatable/data.table/issues/6409 @@ -129,10 +132,8 @@ if [ "${R_OPENMP_ENABLED}" = "0" ]; then echo "*** https://github.com/Rdatatable/data.table/wiki/Installation" echo "*** Continuing installation without OpenMP support..." echo "***" - sed -e "s|@openmp_cflags@||" -e "s|@openmp_libs@||" src/Makevars.in > src/Makevars -else - sed -e "s|@openmp_cflags@|${R_OPENMP_CFLAGS}|" -e "s|@openmp_libs@|${R_OPENMP_LIBS}|" src/Makevars.in > src/Makevars fi +sed -e "s|@openmp_cflags@|${R_OPENMP_CFLAGS}|" -e "s|@openmp_libs@|${R_OPENMP_LIBS}|" src/Makevars.in > src/Makevars # retain user supplied PKG_ env variables, #4664. See comments in Makevars.in too. sed -e "s|@PKG_CFLAGS@|$PKG_CFLAGS|" src/Makevars > src/Makevars.tmp && mv src/Makevars.tmp src/Makevars From 4bd336f9521723107a061a5b980679ebf4a8d969 Mon Sep 17 00:00:00 2001 From: Ivan K Date: Tue, 10 Dec 2024 11:04:50 +0300 Subject: [PATCH 5/5] configure: test a real OpenMP operation It turns out it's possible for omp_get_num_threads() to succeed while failing to load the shared library that does more involved OpenMP operations. --- tools/check-openmp-flags.R | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/tools/check-openmp-flags.R b/tools/check-openmp-flags.R index 4131df732..c2332593d 100644 --- a/tools/check-openmp-flags.R +++ b/tools/check-openmp-flags.R @@ -18,14 +18,14 @@ writeLines(" #ifdef _OPENMP #include #endif -void test_openmp(int * numprocs) { - *numprocs = +void test_openmp(int * result) { + int sum = 0; #ifdef _OPENMP - omp_get_max_threads() -#else - 0 + // Have to test an actual OpenMP operation: simpler tests may succeed for a broken configuration, #6642 + #pragma omp parallel for reduction(+:sum) num_threads(2) + for (int i = 1; i <= 2; ++i) sum += i; #endif - ; + *result = sum; } ", "test.c") @@ -33,7 +33,8 @@ void test_openmp(int * numprocs) { stopifnot(tools::Rcmd("SHLIB --preclean test.c") == 0) dyn.load(paste0("test", .Platform$dynlib.ext)) -success <- .C("test_openmp", ans = integer(1))$ans > 0 - -cat(sprintf("Result: %s\n", if (success) "yes" else "no")) -q("no", status = !success) +ans <- .C("test_openmp", ans = integer(1))$ans +desired <- 3L +cat(sprintf("Test result: %d (must be %d; should be 0 for disabled OpenMP)\n", ans, desired)) +stopifnot(`Test failed` = identical(ans, desired)) +cat("Success\n")