From 22cdaf67f926fd0fb0204ef8084372780acc0a73 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Fri, 7 Oct 2022 17:35:16 +0900 Subject: [PATCH 1/3] ARROW-15678: [C++] Add support for -DCMAKE_BUILD_TYPE=MinSizeRel If we build with -DCMAKE_BUILD_TYPE=MinSizeRel, our SIMD related code may violate the one-definition-rule. See also ARROW-15664 and https://issues.apache.org/jira/browse/ARROW-15678?focusedCommentId=17613909#comment-17613909 for details. This change prevents the one-definition-rule violation by forcing inlining as much as possible. --- cpp/cmake_modules/SetupCxxFlags.cmake | 3 +++ dev/tasks/r/github.macos-linux.local.yml | 6 ++++++ dev/tasks/tasks.yml | 8 +++++++- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake index a9e23be538614..cef4eb0b1617d 100644 --- a/cpp/cmake_modules/SetupCxxFlags.cmake +++ b/cpp/cmake_modules/SetupCxxFlags.cmake @@ -683,6 +683,9 @@ elseif("${CMAKE_BUILD_TYPE}" STREQUAL "PROFILE_GEN") elseif("${CMAKE_BUILD_TYPE}" STREQUAL "PROFILE_BUILD") set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${C_FLAGS_PROFILE_BUILD}") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CXX_FLAGS_PROFILE_BUILD}") +elseif("${CMAKE_BUILD_TYPE}" STREQUAL "MINSIZEREL") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${C_FLAGS_MINSIZEREL}") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CXX_FLAGS_MINSIZEREL}") else() message(FATAL_ERROR "Unknown build type: ${CMAKE_BUILD_TYPE}") endif() diff --git a/dev/tasks/r/github.macos-linux.local.yml b/dev/tasks/r/github.macos-linux.local.yml index 27c67657ba4d4..aaefafdce16d6 100644 --- a/dev/tasks/r/github.macos-linux.local.yml +++ b/dev/tasks/r/github.macos-linux.local.yml @@ -23,6 +23,12 @@ jobs: autobrew: name: "install from local source" runs-on: {{ "${{ matrix.os }}" }} + {% if env is defined %} + env: + {% for key, value in env.items() %} + {{ key }}: "{{ value }}" + {% endfor %} + {% endif %} strategy: fail-fast: false matrix: diff --git a/dev/tasks/tasks.yml b/dev/tasks/tasks.yml index 1fe42168a4914..2cad64296bebc 100644 --- a/dev/tasks/tasks.yml +++ b/dev/tasks/tasks.yml @@ -1314,6 +1314,13 @@ tasks: ci: github template: r/github.macos-linux.local.yml + test-r-install-local-minsizerel: + ci: github + template: r/github.macos-linux.local.yml + params: + env: + CMAKE_BUILD_TYPE: MinSizeRel + test-r-devdocs: ci: github template: r/github.devdocs.yml @@ -1355,7 +1362,6 @@ tasks: ci: github template: r/github.linux.offline.build.yml - test-r-rhub-debian-gcc-release-custom-ccache: ci: azure template: r/azure.linux.yml From 1f4d567127ad2cc607af6136da31dd8e6ec8b795 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Fri, 7 Oct 2022 17:43:46 +0900 Subject: [PATCH 2/3] Always optimize SIMD codes --- cpp/src/parquet/CMakeLists.txt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt index 1244ed662975b..a8a82b3390a4c 100644 --- a/cpp/src/parquet/CMakeLists.txt +++ b/cpp/src/parquet/CMakeLists.txt @@ -181,8 +181,9 @@ if(ARROW_HAVE_RUNTIME_AVX2) # AVX2 is used as a proxy for BMI2. list(APPEND PARQUET_SRCS level_comparison_avx2.cc level_conversion_bmi2.cc) set_source_files_properties(level_comparison_avx2.cc - PROPERTIES SKIP_PRECOMPILE_HEADERS ON COMPILE_FLAGS - "${ARROW_AVX2_FLAG}") + PROPERTIES SKIP_PRECOMPILE_HEADERS ON + COMPILE_FLAGS + "${ARROW_AVX2_FLAG} ${CXX_FLAGS_RELEASE}") # WARNING: DO NOT BLINDLY COPY THIS CODE FOR OTHER BMI2 USE CASES. # This code is always guarded by runtime dispatch which verifies # BMI2 is present. For a very small number of CPUs AVX2 does not @@ -190,7 +191,8 @@ if(ARROW_HAVE_RUNTIME_AVX2) set_source_files_properties(level_conversion_bmi2.cc PROPERTIES SKIP_PRECOMPILE_HEADERS ON COMPILE_FLAGS - "${ARROW_AVX2_FLAG} -DARROW_HAVE_BMI2 -mbmi2") + "${ARROW_AVX2_FLAG} -DARROW_HAVE_BMI2 ${CXX_FLAGS_RELEASE}" + ) endif() if(PARQUET_REQUIRE_ENCRYPTION) From 1d2a984cd48d6a1a7c1a29bb1bf22c65187b1405 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Mon, 10 Oct 2022 06:16:24 +0900 Subject: [PATCH 3/3] Refer ARROW-15665 and ARROW-15678 in comment --- cpp/src/parquet/CMakeLists.txt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt index a8a82b3390a4c..f20a83e212465 100644 --- a/cpp/src/parquet/CMakeLists.txt +++ b/cpp/src/parquet/CMakeLists.txt @@ -180,6 +180,10 @@ set(PARQUET_SRCS if(ARROW_HAVE_RUNTIME_AVX2) # AVX2 is used as a proxy for BMI2. list(APPEND PARQUET_SRCS level_comparison_avx2.cc level_conversion_bmi2.cc) + # We need CXX_FLAGS_RELEASE here to prevent the one-definition-rule + # violation with -DCMAKE_BUILD_TYPE=MinSizeRel. CXX_FLAGS_RELEASE + # will force inlining as much as possible. + # See also: ARROW-15664 and ARROW-15678 set_source_files_properties(level_comparison_avx2.cc PROPERTIES SKIP_PRECOMPILE_HEADERS ON COMPILE_FLAGS @@ -188,6 +192,11 @@ if(ARROW_HAVE_RUNTIME_AVX2) # This code is always guarded by runtime dispatch which verifies # BMI2 is present. For a very small number of CPUs AVX2 does not # imply BMI2. + # + # We need CXX_FLAGS_RELEASE here to prevent the one-definition-rule + # violation with -DCMAKE_BUILD_TYPE=MinSizeRel. CXX_FLAGS_RELEASE + # will force inlining as much as possible. + # See also: ARROW-15664 and ARROW-15678 set_source_files_properties(level_conversion_bmi2.cc PROPERTIES SKIP_PRECOMPILE_HEADERS ON COMPILE_FLAGS