-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
cmake: Allow specifying optimization level in COMMON_OPT #3748
base: develop
Are you sure you want to change the base?
Conversation
Maybe it is worth playing warning, my rationale is that -Os has a practical purpose always when used, but say -Ofast guarantees numeric faults. |
I'm not sure I understand your comment. |
PR is not wrong, just that probably needs some discussion/consideration. |
I'm happy to discuss. But I don't understand which changes you are proposing. |
I'm not sure why one of the checks of this PR was failing: IIUC, that check uses |
On the actual topic of your PR, I am unsure if it would be useful to change the well-defined CMAKE default behaviour for Release builds years after adding cmake support. Perhaps it might be more useful to just handle the case where the user declared no build type at all (as a novice user might), which currently would result in no |
Afaict, it is not completely uncommon that projects override the flags in At the same time, it might be surprising for users if optimization flags that they pass with the Should the build rules check if there are any |
7db2983
to
d528a26
Compare
I updated the PR to leave cmake's default optimization level alone unless the user explicitly specifies optimization flags in any of the |
d528a26
to
d059eb8
Compare
The default optimization level might be overridden by default settings of cmake.
d059eb8
to
c1a5a71
Compare
I think we set release_with_debuginfo for novice user case which roughly matches makefile behaviour. |
When building with
make
a default optimization level of-O2
is used.See:
https://github.com/xianyi/OpenBLAS/blob/00534523ad999d89945d23b7df0eafc69c31f1b3/Makefile.system#L1551-L1557
By default,
cmake
uses-O3
forRelease
builds currently:https://github.com/Kitware/CMake/blob/v3.24.1/Modules/Compiler/GNU.cmake#L59
This means binaries are built with different optimization levels depending on whether
make
orcmake
is used.Additionally, it is currently not possible to override the optimization level with, e.g.,
-DCOMMON_OPT=-O2
because it appears before theCMAKE_${Language}_FLAGS_RELEASE
flags (which effectively overrides the flag inCOMMON_OPT
).See also: #3740
The proposed change sets the default compiler optimization level that is used with
cmake
to-O2
(i.e., the same that is used bymake
).Additionally, it allows overriding the compiler optimization level with flags in
COMMON_OPT
,CCOMMON_OPT
, andFCOMMON_OPT
.