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

compiler_flags: Change optimization_fast from '-Ofast' to '-O3' #87495

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thughes
Copy link
Contributor

@thughes thughes commented Mar 21, 2025

'-Ofast' enables optimizations that are not standards compliant, which can
produce unexpected results unless used carefully.

When building with clang 19 and newer, it warns:

clang: error: argument '-Ofast' is deprecated; use '-O3 -ffast-math' for
the same behavior, or '-O3' to enable only conforming optimizations
[-Werror,-Wdeprecated-ofast]

Relevant discussion:

@thughes
Copy link
Contributor Author

thughes commented Mar 21, 2025

There are other possibilities here, such as disable the warning for clang, only change clang to use -O3, or change clang to use -O3 -ffast-math to be similar to gcc.

However, it seems like we want the meaning of this flag to be the same across all compilers, so I think it makes sense for whatever we do to be consistent. If we want to keep the unsafe optimizations, it might be good to rename it to something like optimization_fast_unsafe so people pay attention when using it.

@@ -21,7 +21,7 @@ endif()
set_compiler_property(PROPERTY optimization_speed -O2)
set_compiler_property(PROPERTY optimization_size -Os)
set_compiler_property(PROPERTY optimization_size_aggressive -Oz)
set_compiler_property(PROPERTY optimization_fast -Ofast)
set_compiler_property(PROPERTY optimization_fast -O3)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we choose to make this change, I'll add a context comment here so it doesn't accidentally get changed back in the future.

@thughes thughes marked this pull request as ready for review March 21, 2025 20:31
@zephyrbot zephyrbot added area: Toolchains Toolchains size: XS A PR changing only a single line of code area: Build System labels Mar 21, 2025
@thughes thughes force-pushed the push-sppopnmtpnxw branch from 9919c29 to a52b9d4 Compare March 21, 2025 22:04
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

PR says changing for clang but is changing for gcc, clang changes should go in the clang file as per https://github.com/zephyrproject-rtos/zephyr/blob/a52b9d4084133edbdf547f308f5cb349a764fcc3/cmake/compiler/clang/compiler_flags.cmake#L1-L4]

GCC has no plans to deprecate this flag https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115990

'-Ofast' enables optimizations that are not standards compliant, which can
produce unexpected results unless used carefully.

When building with clang 19 and newer, it warns:

clang: error: argument '-Ofast' is deprecated; use '-O3 -ffast-math' for
the same behavior, or '-O3' to enable only conforming optimizations
[-Werror,-Wdeprecated-ofast]

Relevant discussion:

* llvm/llvm-project#98736
* https://discourse.llvm.org/t/rfc-deprecate-ofast/78687/109
* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115990

Signed-off-by: Tom Hughes <[email protected]>
@thughes thughes force-pushed the push-sppopnmtpnxw branch from a52b9d4 to e42bd38 Compare March 24, 2025 17:30
@thughes
Copy link
Contributor Author

thughes commented Mar 25, 2025

I think we want to be consistent on this on all compilers.

-Ofast is not standards compliant on both gcc and clang. We probably don't want people to use it unless they're really sure that it's safe for the code they're enabling it on. The name optimization_fast suggests that it's just faster, not necessarily unsafe. We could rename it to optimization_fast_unsafe to make it clear that it's not compliant. Or we could just choose to make optimization_fast be something that is compliant like I proposed in this change. (It's not clear whether the unsafe aspect was taken into account when it was enabled for the code that is currently using it.)

@thughes thughes requested a review from nordicjm March 25, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: Toolchains Toolchains size: XS A PR changing only a single line of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants