Skip to content

[libc] Use ${libc_opt_high_flag} instead of -O3 #123233

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 7, 2025
Merged

[libc] Use ${libc_opt_high_flag} instead of -O3 #123233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
150f6a5
[libc] Use `${libc_opt_high_flag}` instead of `-O3`
petrhosek Jan 16, 2025
0ca085c
Avoid applying compile options to the linker
petrhosek Feb 5, 2025
5ea4d2a
Pass common compile options to the linker
petrhosek Feb 6, 2025
3ac91a8
Actually set link options
petrhosek Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion libc/cmake/modules/LLVMLibCTestRules.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,8 @@ function(create_libc_unittest fq_target_name)

_get_common_test_compile_options(compile_options "${LIBC_UNITTEST_C_TEST}"
"${LIBC_UNITTEST_FLAGS}")
# TODO: Ideally we would have a separate function for link options.
set(link_options ${compile_options})
list(APPEND compile_options ${LIBC_UNITTEST_COMPILE_OPTIONS})

if(SHOW_INTERMEDIATE_OBJECTS)
Expand Down Expand Up @@ -272,7 +274,7 @@ function(create_libc_unittest fq_target_name)
target_include_directories(${fq_build_target_name} SYSTEM PRIVATE ${LIBC_INCLUDE_DIR})
target_include_directories(${fq_build_target_name} PRIVATE ${LIBC_SOURCE_DIR})
target_compile_options(${fq_build_target_name} PRIVATE ${compile_options})
target_link_options(${fq_build_target_name} PRIVATE ${compile_options})
target_link_options(${fq_build_target_name} PRIVATE ${link_options})

if(NOT LIBC_UNITTEST_CXX_STANDARD)
set(LIBC_UNITTEST_CXX_STANDARD ${CMAKE_CXX_STANDARD})
Expand Down
2 changes: 1 addition & 1 deletion libc/src/math/generic/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ add_entrypoint_object(
libc.src.__support.macros.optimization
libc.src.__support.macros.properties.types
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to avoid having to spell this out again and again and again for each entry point? I don't see why the math.h functions should be built with a different optimization level than the rest of the libc. The codebase should use the same -O* flag consistently everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with removing the separate optimization level for math functions, but we do need to add build bots with different optimization levels to make sure that math functions are still correct.

Copy link
Member

Choose a reason for hiding this comment

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

Why would they be incorrect based on optimization level?

Copy link
Contributor

@lntue lntue Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

-O3 was notorious for aggressive optimizations, especially with gcc. Nowadays -O2 and -O3 are probably close, but there are still differences. It's probably safer to make sure different optimizations do not change the floating point computations' outcomes.

Copy link
Member

Choose a reason for hiding this comment

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

We should key off what optimization levels are set for CMAKE_BUILD_TYPE and not override the users intent. If users hack up their configs to use different optimization levels, let them report bugs upstream to us.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I plan to remove all of these explicit flags in a follow up patch once we have a bot set up. This change was supposed to be just an NFC cleanup, but as you can see in #123233 (comment), it already uncovered an issue to how we handle flags on Windows.

)

add_entrypoint_object(
Expand Down
2 changes: 1 addition & 1 deletion libc/test/src/__support/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ add_libc_test(
libc.src.stdlib.srand
libc.src.string.memset
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
UNIT_TEST_ONLY
# Aligned Allocation is not supported in hermetic builds.
)
Expand Down
6 changes: 3 additions & 3 deletions libc/test/src/math/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1597,7 +1597,7 @@ add_fp_unittest(
libc.src.math.sqrtf
libc.src.__support.FPUtil.generic.sqrt
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
)

add_fp_unittest(
Expand All @@ -1613,7 +1613,7 @@ add_fp_unittest(
libc.src.math.sqrt
libc.src.__support.FPUtil.generic.sqrt
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
)

add_fp_unittest(
Expand All @@ -1629,7 +1629,7 @@ add_fp_unittest(
libc.src.math.sqrtl
libc.src.__support.FPUtil.generic.sqrt
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
)

add_fp_unittest(
Expand Down
2 changes: 1 addition & 1 deletion libc/test/src/math/exhaustive/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ add_fp_unittest(
SRCS
hypotf_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
.exhaustive_test
libc.src.math.hypotf
Expand Down
8 changes: 4 additions & 4 deletions libc/test/src/math/smoke/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2990,7 +2990,7 @@ add_fp_unittest(
DEPENDS
libc.src.__support.FPUtil.generic.sqrt
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
)

add_fp_unittest(
Expand All @@ -3004,7 +3004,7 @@ add_fp_unittest(
DEPENDS
libc.src.__support.FPUtil.generic.sqrt
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
)

add_fp_unittest(
Expand All @@ -3018,7 +3018,7 @@ add_fp_unittest(
DEPENDS
libc.src.__support.FPUtil.generic.sqrt
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
)

add_fp_unittest(
Expand All @@ -3035,7 +3035,7 @@ add_fp_unittest(
libc.src.math.sqrtf128
libc.src.__support.FPUtil.generic.sqrt
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
)

add_fp_unittest(
Expand Down
16 changes: 8 additions & 8 deletions libc/test/src/stdfix/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ foreach(suffix IN ITEMS hr r lr hk k lk)
SRCS
abs${suffix}_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
libc.src.stdfix.abs${suffix}
libc.src.__support.fixed_point.fx_bits
Expand All @@ -31,7 +31,7 @@ foreach(suffix IN ITEMS uhr ur ulr uhk uk)
SRCS
sqrt${suffix}_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
libc.src.stdfix.sqrt${suffix}
libc.src.__support.CPP.bit
Expand All @@ -52,7 +52,7 @@ foreach(suffix IN ITEMS hr r lr hk k lk uhr ur ulr uhk uk ulk)
SRCS
round${suffix}_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
libc.src.stdfix.round${suffix}
libc.src.__support.fixed_point.fx_bits
Expand All @@ -67,7 +67,7 @@ foreach(suffix IN ITEMS hr r lr hk k lk uhr ur ulr uhk uk ulk)
SRCS
${suffix}bits_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
libc.src.stdfix.${suffix}bits
libc.src.__support.CPP.bit
Expand All @@ -84,7 +84,7 @@ add_libc_test(
SRCS
uhksqrtus_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
libc.src.stdfix.uhksqrtus
libc.src.__support.CPP.bit
Expand All @@ -103,7 +103,7 @@ add_libc_test(
SRCS
uksqrtui_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
libc.src.stdfix.uksqrtui
libc.src.__support.CPP.bit
Expand All @@ -122,7 +122,7 @@ add_libc_test(
SRCS
exphk_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
libc.src.stdfix.exphk
libc.src.math.exp
Expand All @@ -140,7 +140,7 @@ add_libc_test(
SRCS
expk_test.cpp
COMPILE_OPTIONS
-O3
${libc_opt_high_flag}
DEPENDS
libc.src.stdfix.expk
libc.src.math.exp
Expand Down
2 changes: 1 addition & 1 deletion libc/utils/MPFRWrapper/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ if(LIBC_TESTS_CAN_USE_MPFR)
_get_common_test_compile_options(compile_options "" "")
# mpfr/gmp headers do not work with -ffreestanding flag.
list(REMOVE_ITEM compile_options "-ffreestanding")
target_compile_options(libcMPFRWrapper PRIVATE -O3 ${compile_options})
target_compile_options(libcMPFRWrapper PRIVATE ${libc_opt_high_flag} ${compile_options})
add_dependencies(
libcMPFRWrapper
libc.src.__support.CPP.array
Expand Down