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

petrhosek · 2025-01-16T20:00:31Z

This is preferable since ${libc_opt_high_flag} will be set correctly for the compiler used.

This is preferable since `${libc_opt_high_flag}` will be set correctly for the compiler used.

llvmbot · 2025-01-16T20:01:16Z

@llvm/pr-subscribers-libc

Author: Petr Hosek (petrhosek)

Changes

This is preferable since ${libc_opt_high_flag} will be set correctly for the compiler used.

Full diff: https://github.com/llvm/llvm-project/pull/123233.diff

7 Files Affected:

(modified) libc/src/math/generic/CMakeLists.txt (+1-1)
(modified) libc/test/src/__support/CMakeLists.txt (+1-1)
(modified) libc/test/src/math/CMakeLists.txt (+3-3)
(modified) libc/test/src/math/exhaustive/CMakeLists.txt (+1-1)
(modified) libc/test/src/math/smoke/CMakeLists.txt (+4-4)
(modified) libc/test/src/stdfix/CMakeLists.txt (+8-8)
(modified) libc/utils/MPFRWrapper/CMakeLists.txt (+1-1)

diff --git a/libc/src/math/generic/CMakeLists.txt b/libc/src/math/generic/CMakeLists.txt
index 0e57051807b332..14e63d6cc13956 100644
--- a/libc/src/math/generic/CMakeLists.txt
+++ b/libc/src/math/generic/CMakeLists.txt
@@ -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}
 )
 
 add_entrypoint_object(
diff --git a/libc/test/src/__support/CMakeLists.txt b/libc/test/src/__support/CMakeLists.txt
index aeb8edf305d059..8d175e857fcd11 100644
--- a/libc/test/src/__support/CMakeLists.txt
+++ b/libc/test/src/__support/CMakeLists.txt
@@ -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.
 )
diff --git a/libc/test/src/math/CMakeLists.txt b/libc/test/src/math/CMakeLists.txt
index ae8518ee4b4cc1..5146b9624d4786 100644
--- a/libc/test/src/math/CMakeLists.txt
+++ b/libc/test/src/math/CMakeLists.txt
@@ -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(
@@ -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(
@@ -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(
diff --git a/libc/test/src/math/exhaustive/CMakeLists.txt b/libc/test/src/math/exhaustive/CMakeLists.txt
index 423c3b7a8bfd11..b1927dbc19a3bd 100644
--- a/libc/test/src/math/exhaustive/CMakeLists.txt
+++ b/libc/test/src/math/exhaustive/CMakeLists.txt
@@ -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
diff --git a/libc/test/src/math/smoke/CMakeLists.txt b/libc/test/src/math/smoke/CMakeLists.txt
index e23e7f41222d4a..31f8ac40cc19fb 100644
--- a/libc/test/src/math/smoke/CMakeLists.txt
+++ b/libc/test/src/math/smoke/CMakeLists.txt
@@ -2990,7 +2990,7 @@ add_fp_unittest(
   DEPENDS
     libc.src.__support.FPUtil.generic.sqrt
   COMPILE_OPTIONS
-    -O3
+    ${libc_opt_high_flag}
 )
 
 add_fp_unittest(
@@ -3004,7 +3004,7 @@ add_fp_unittest(
   DEPENDS
     libc.src.__support.FPUtil.generic.sqrt
   COMPILE_OPTIONS
-    -O3
+    ${libc_opt_high_flag}
 )
 
 add_fp_unittest(
@@ -3018,7 +3018,7 @@ add_fp_unittest(
   DEPENDS
     libc.src.__support.FPUtil.generic.sqrt
   COMPILE_OPTIONS
-    -O3
+    ${libc_opt_high_flag}
 )
 
 add_fp_unittest(
@@ -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(
diff --git a/libc/test/src/stdfix/CMakeLists.txt b/libc/test/src/stdfix/CMakeLists.txt
index 60e38c9098c387..1b3a1b965fc6c9 100644
--- a/libc/test/src/stdfix/CMakeLists.txt
+++ b/libc/test/src/stdfix/CMakeLists.txt
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/libc/utils/MPFRWrapper/CMakeLists.txt b/libc/utils/MPFRWrapper/CMakeLists.txt
index 0101c9f3990822..792ed5a9a2211a 100644
--- a/libc/utils/MPFRWrapper/CMakeLists.txt
+++ b/libc/utils/MPFRWrapper/CMakeLists.txt
@@ -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

jhuber6 · 2025-01-16T20:11:19Z

Do we want to override debug builds?

petrhosek · 2025-01-16T20:23:12Z

Do we want to override debug builds?

We talked about it in he libc meeting today and agreed that we shouldn't be overriding the flag and instead rely on whatever user sets in CMake (except for cases where -O3 is absolutely necessary). I plan on doing that in a follow up PR, this change is just an initial cleanup.

lntue · 2025-01-17T17:09:23Z

Can you take a look at the Windows presubmit failure?

nickdesaulniers · 2025-01-17T17:12:07Z

libc/src/math/generic/CMakeLists.txt

@@ -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}


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.

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.

Why would they be incorrect based on optimization level?

-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.

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.

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.

petrhosek · 2025-01-17T18:22:40Z

Can you take a look at the Windows presubmit failure?

Looks like the optimization flag is being passed to both the compiler and the linker. That's fine on Linux or macOS since clang is invoked as the linker and it'll consume the optimization flag, but on Windows we invoke the linker directly and lld-link (or link) doesn't handle /O2. I don't know why this works now, it's possible that lld-link just consumes and ignores -O3.

Compile options aren't the same as link options and on some platforms like Windows where the linker is invoked directly they're distinctly different.

nickdesaulniers · 2025-02-05T17:06:47Z

One thing to triple check is that when cross compiling (-DLIBC_TARGET_TRIPLE=aarch64-linux-gnu) and using lld (-DLLVM_ENABLE_LLD=ON), do we still pass --target=aarch64-linux-gnu to the linker?

petrhosek · 2025-02-07T20:25:08Z

Looks like all the tests are passing now.

This is preferable since `${libc_opt_high_flag}` will be set correctly for the compiler used.

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

150f6a5

This is preferable since `${libc_opt_high_flag}` will be set correctly for the compiler used.

petrhosek added the libc label Jan 16, 2025

petrhosek requested a review from lntue January 16, 2025 20:00

nickdesaulniers reviewed Jan 17, 2025

View reviewed changes

Avoid applying compile options to the linker

0ca085c

Compile options aren't the same as link options and on some platforms like Windows where the linker is invoked directly they're distinctly different.

petrhosek added 2 commits February 6, 2025 09:08

Pass common compile options to the linker

5ea4d2a

Actually set link options

3ac91a8

lntue approved these changes Feb 7, 2025

View reviewed changes

petrhosek merged commit 6dbe542 into llvm:main Feb 7, 2025
14 checks passed

Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025

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

e3ed689

This is preferable since `${libc_opt_high_flag}` will be set correctly for the compiler used.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

petrhosek commented Jan 16, 2025

Uh oh!

llvmbot commented Jan 16, 2025

Uh oh!

jhuber6 commented Jan 16, 2025

Uh oh!

petrhosek commented Jan 16, 2025

Uh oh!

lntue commented Jan 17, 2025

Uh oh!

nickdesaulniers Jan 17, 2025

Uh oh!

lntue Jan 17, 2025

Uh oh!

nickdesaulniers Jan 17, 2025

Uh oh!

lntue Jan 17, 2025 •

edited

Loading

Uh oh!

nickdesaulniers Jan 17, 2025

Uh oh!

petrhosek Jan 17, 2025

Uh oh!

petrhosek commented Jan 17, 2025

Uh oh!

nickdesaulniers commented Feb 5, 2025

Uh oh!

petrhosek commented Feb 7, 2025

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

petrhosek commented Jan 16, 2025

Uh oh!

llvmbot commented Jan 16, 2025

Uh oh!

jhuber6 commented Jan 16, 2025

Uh oh!

petrhosek commented Jan 16, 2025

Uh oh!

lntue commented Jan 17, 2025

Uh oh!

nickdesaulniers Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

lntue Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

lntue Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

petrhosek Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

petrhosek commented Jan 17, 2025

Uh oh!

nickdesaulniers commented Feb 5, 2025

Uh oh!

petrhosek commented Feb 7, 2025

Uh oh!

Uh oh!

Uh oh!

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

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

lntue Jan 17, 2025 •

edited

Loading