Skip to content
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

[C++] Cannot change optimization level using ARROW_CXXFLAGS #35870

Closed
pitrou opened this issue Jun 1, 2023 · 6 comments · Fixed by #35924
Closed

[C++] Cannot change optimization level using ARROW_CXXFLAGS #35870

pitrou opened this issue Jun 1, 2023 · 6 comments · Fixed by #35924
Assignees
Milestone

Comments

@pitrou
Copy link
Member

pitrou commented Jun 1, 2023

Describe the bug, including details regarding any error messages, version, and platform.

ARROW_CXXFLAGS used to allow changing the optimization level. However, this now gets ignored as the default optimization flags get appended after the ARROW_CXXFLAGS.

For example, the conda-cpp-valgrind job produces the following CMake configuration:

-- CMAKE_C_FLAGS: -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /opt/conda/envs/arrow/include -fuse-ld=gold  -Wall -fno-semantic-interposition -msse4.2 -Og
-- CMAKE_CXX_FLAGS:  -Wno-noexcept-type -fvisibility-inlines-hidden -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /opt/conda/envs/arrow/include -fdiagnostics-color=always -fuse-ld=gold  -Wall -fno-semantic-interposition -msse4.2 -Og
-- CMAKE_C_FLAGS_DEBUG: -g -O0 -ggdb
-- CMAKE_CXX_FLAGS_DEBUG: -g -O0 -ggdb

... and then the following compiler commands:

[1/684] /opt/conda/envs/arrow/bin/ccache /opt/conda/envs/arrow/bin/x86_64-conda-linux-gnu-c++ -DARROW_EXPORTING -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_HDFS -DARROW_VALGRIND -DARROW_WITH_BACKTRACE -DARROW_WITH_BROTLI -DARROW_WITH_BZ2 -DARROW_WITH_LZ4 -DARROW_WITH_RE2 -DARROW_WITH_SNAPPY -DARROW_WITH_TIMING_TESTS -DARROW_WITH_UTF8PROC -DARROW_WITH_ZLIB -DARROW_WITH_ZSTD -DURI_STATIC_BUILD -I/build/cpp/src -I/arrow/cpp/src -I/arrow/cpp/src/generated -isystem /arrow/cpp/thirdparty/flatbuffers/include -isystem /arrow/cpp/thirdparty/hadoop/include -Wno-noexcept-type -fvisibility-inlines-hidden -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /opt/conda/envs/arrow/include -fdiagnostics-color=always -fuse-ld=gold  -Wall -fno-semantic-interposition -msse4.2 -Og -g -O0 -ggdb -std=c++17 -fPIC -pthread -MD -MT src/arrow/CMakeFiles/arrow_objlib.dir/util/bitmap_builders.cc.o -MF src/arrow/CMakeFiles/arrow_objlib.dir/util/bitmap_builders.cc.o.d -o src/arrow/CMakeFiles/arrow_objlib.dir/util/bitmap_builders.cc.o -c /arrow/cpp/src/arrow/util/bitmap_builders.cc

(notice how -O0 overrides -Og by coming after it)

ARROW_CXXFLAGS should ideally override any default settings.

Component(s)

C++

@pitrou
Copy link
Member Author

pitrou commented Jun 1, 2023

cc @kou @assignUser

@pitrou
Copy link
Member Author

pitrou commented Jun 1, 2023

It's not clear to me what the point is of setting the optimization level in DEBUG_FLAGS. CMake should probably disable optimizations in debug mode by default?

@assignUser
Copy link
Member

IIRC it does but I have to check what we do with the flags and what the default is 👍

@kou
Copy link
Member

kou commented Jun 1, 2023

This is expected.
We should use CMAKE_CXX_FLAGS_${CONFIG} such as CMAKE_CXX_FLAGS_RELEASE instead of ARROW_CXXFLAGS for optimization flags.
See also: #15022

@pitrou
Copy link
Member Author

pitrou commented Jun 3, 2023

Same problem: it doesn't work.
For example I pass -DCMAKE_CXX_FLAGS_DEBUG="-Og" and I get command lines such as:

[68/764] ccache /home/antoine/mambaforge/envs/pyarrow/bin/g++ -DARROW_EXPORTING -DARROW_EXTRA_ERROR_CONTEXT -DARROW_HAVE_SSE4_2 -DARROW_HDFS -DARROW_MIMALLOC -DARROW_S3_HAS_CRT -DARROW_WITH_BACKTRACE -DARROW_WITH_BROTLI -DARROW_WITH_BZ2 -DARROW_WITH_LZ4 -DARROW_WITH_RE2 -DARROW_WITH_SNAPPY -DARROW_WITH_TIMING_TESTS -DARROW_WITH_UTF8PROC -DARROW_WITH_ZLIB -DARROW_WITH_ZSTD -DAWS_AUTH_USE_IMPORT_EXPORT -DAWS_CAL_USE_IMPORT_EXPORT -DAWS_CHECKSUMS_USE_IMPORT_EXPORT -DAWS_COMMON_USE_IMPORT_EXPORT -DAWS_COMPRESSION_USE_IMPORT_EXPORT -DAWS_CRT_CPP_USE_IMPORT_EXPORT -DAWS_EVENT_STREAM_USE_IMPORT_EXPORT -DAWS_HTTP_USE_IMPORT_EXPORT -DAWS_IO_USE_IMPORT_EXPORT -DAWS_MQTT_USE_IMPORT_EXPORT -DAWS_MQTT_WITH_WEBSOCKETS -DAWS_S3_USE_IMPORT_EXPORT -DAWS_SDKUTILS_USE_IMPORT_EXPORT -DAWS_SDK_VERSION_MAJOR=1 -DAWS_SDK_VERSION_MINOR=11 -DAWS_SDK_VERSION_PATCH=68 -DAWS_USE_EPOLL -DURI_STATIC_BUILD -I/build/build-test/src -I/home/antoine/arrow/dev/cpp/src -I/home/antoine/arrow/dev/cpp/src/generated -isystem /home/antoine/arrow/dev/cpp/thirdparty/flatbuffers/include -isystem /home/antoine/arrow/dev/cpp/thirdparty/hadoop/include -isystem /home/antoine/mambaforge/envs/pyarrow/include -isystem /build/build-test/jemalloc_ep-prefix/src -isystem /build/build-test/mimalloc_ep/src/mimalloc_ep/include/mimalloc-2.0 -Wno-noexcept-type  -fdiagnostics-color=always -fuse-ld=gold  -Wall -Wno-conversion -Wno-sign-conversion -Wunused-result -Wdate-time -fno-semantic-interposition -msse4.2  -Og -Werror -O0 -ggdb -std=c++17 -fPIC -pthread -DS2N_KYBER512R3_AVX2_BMI2 -DS2N_STACKTRACE -DS2N_CPUID_AVAILABLE -DS2N_FEATURES_AVAILABLE -fPIC -DS2N_FALL_THROUGH_SUPPORTED -DS2N___RESTRICT__SUPPORTED -DS2N_MADVISE_SUPPORTED -DS2N_CLONE_SUPPORTED -DS2N_LIBCRYPTO_SUPPORTS_EVP_MD5_SHA1_HASH -DS2N_LIBCRYPTO_SUPPORTS_EVP_RC4 -DS2N_LIBCRYPTO_SUPPORTS_EVP_MD_CTX_SET_PKEY_CTX -MD -MT src/arrow/CMakeFiles/arrow_objlib.dir/util/bitmap.cc.o -MF src/arrow/CMakeFiles/arrow_objlib.dir/util/bitmap.cc.o.d -o src/arrow/CMakeFiles/arrow_objlib.dir/util/bitmap.cc.o -c /home/antoine/arrow/dev/cpp/src/arrow/util/bitmap.cc

Notice how -O0 comes after -Og on the command line.

@kou
Copy link
Member

kou commented Jun 6, 2023

Oh, it's not expected.
I'll fix it.

kou added a commit to kou/arrow that referenced this issue Jun 6, 2023
…h CMAKE_CXX_FLAGS_DEBUG

`-DCMAKE_CXX_FLAGS_RELEASE=-O...` works with apache#15022 but
`-DCMAKE_CXX_FLAGS_DEBUG=-O...` and
`-DCMAKE_CXX_FLAGS_RELWITHDEBINFO=-O...` don't work. Because we always
specify `-O0` for Debug and RelWithDebInfo.

These changes don't specify optimization levels for Debug and
RelWithDebInfo because optimization flags we specified are the same as
the default one. So user specified `CMAKE_CXX_FLAGS_DEBUG=-O...` and
`CMAKE_CXX_FLAGS_RELWITHDEBINFO=-O...` are used.

In addition, `ARROW_C_FLAGS_${CONFIG}` and `ARROW_CXX_FLAGS_${CONFIG}`
are introduced. Users can also use `-DARROW_CXX_FLAGS_DEBUG=-O...`
instead of `-DCMAKE_CXX_FLAGS_DEBUG=-O...`.
kou added a commit to kou/arrow that referenced this issue Jun 15, 2023
…h CMAKE_CXX_FLAGS_DEBUG

`-DCMAKE_CXX_FLAGS_RELEASE=-O...` works with apache#15022 but
`-DCMAKE_CXX_FLAGS_DEBUG=-O...` and
`-DCMAKE_CXX_FLAGS_RELWITHDEBINFO=-O...` don't work. Because we always
specify `-O0` for Debug and RelWithDebInfo.

These changes don't specify optimization levels for Debug and
RelWithDebInfo because optimization flags we specified are the same as
the default one. So user specified `CMAKE_CXX_FLAGS_DEBUG=-O...` and
`CMAKE_CXX_FLAGS_RELWITHDEBINFO=-O...` are used.

In addition, `ARROW_C_FLAGS_${CONFIG}` and `ARROW_CXX_FLAGS_${CONFIG}`
are introduced. Users can also use `-DARROW_CXX_FLAGS_DEBUG=-O...`
instead of `-DCMAKE_CXX_FLAGS_DEBUG=-O...`.
kou added a commit that referenced this issue Jun 20, 2023
…E_CXX_FLAGS_DEBUG (#35924)

### Rationale for this change

`-DCMAKE_CXX_FLAGS_RELEASE=-O...` works with #15022 but `-DCMAKE_CXX_FLAGS_DEBUG=-O...` and
`-DCMAKE_CXX_FLAGS_RELWITHDEBINFO=-O...` don't work. Because we always specify `-O0` for Debug and RelWithDebInfo.

### What changes are included in this PR?

These changes don't specify optimization levels for Debug and RelWithDebInfo unconditionally. We don't specify optimization levels for RelWithDebInfo the default flags include them. We specify `-O0` for Debug only when the default flags don't include any optimization levels. So user specified `CMAKE_CXX_FLAGS_DEBUG=-O...` and `CMAKE_CXX_FLAGS_RELWITHDEBINFO=-O...` are used.

In addition, `ARROW_C_FLAGS_${CONFIG}` and `ARROW_CXX_FLAGS_${CONFIG}` are introduced. Users can also use `-DARROW_CXX_FLAGS_DEBUG=-O...` instead of `-DCMAKE_CXX_FLAGS_DEBUG=-O...`.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: #35870

Lead-authored-by: Sutou Kouhei <kou@clear-code.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 13.0.0 milestone Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants