cmake: compiler: add double promotion warning #57154
Conversation
XenuIsWatching
requested review from
abrodkin,
evgeniy-paltsev,
tejlmand and
nashif
as code owners
XenuIsWatching
force-pushed
the
warn-double-promotion
branch
from
d6c6213
to
d614946
Compare
There was a problem hiding this comment.
Yes, we definitely want this warning enabled by default. Building picolibc with -Werror=double-promotion yielded dozens of fixes and made the resulting library free of double operations when using only float values.
However, fixes to make this work should probably land in separate PR/PRs, but having them appear here first so that we can let CI check to make sure they're sufficient seems like a good plan. Should we mark this as DNM until then?
Once the patches that fix the promotion bugs have landed, I'd be happy to approve this.
Agreed, it should be marked as DNM until all the little double promotion bugs are squashed in seperate PRs. My only worry is with modules that have double-promotion issues, that's what stopped me the last time I try to integrated this flag by default to in zephyr, but some may of been fixed since then.... speaking of which, we got our first hit (it's just that with how large and expansive zephyr is, it's gonna be easier and quicker for me to rely on the server running the test cases, as my local machine is gonna take a loooong time to get through them all with this flag enabled) |
XenuIsWatching
changed the title
XenuIsWatching
force-pushed
the
warn-double-promotion
branch
from
d614946
to
b4f9032
Compare
XenuIsWatching
requested review from
rlubos,
tbursztyka and
nandojve
as code owners
Yeah, it's tedious but there is a pretty reasonable process. Post a PR to the module repo, then post a PR to the zephyr repo that pulls the module branch. It's documented somewhere even ... |
XenuIsWatching
force-pushed
the
warn-double-promotion
branch
from
2c8f63e
to
9f938e7
Compare
XenuIsWatching
changed the title
|
@fabiobaltieri Can you merge this into zephyr-testing again? I really hope this is the last time as it now appears all the checkboxes are now checked 😄 |
|
@XenuIsWatching hey welcome back! There you have it: https://github.com/zephyrproject-rtos/zephyr-testing/tree/vfabio-57154-branch |
In C, a literal value like "2.5" is considered a double. When you
multiply a float by a double, implicit promotion occurs, performing
the calculation in 64 bits. This unnecessarily consumes CPU power on
MCUs.
[118/166] Building C object .../sc_dstrx3.c.obj
In file included from .../logging/log.h:11,
from .../sc_dstrx3.c:9:
.../sc_dstrx3.c: In function 'sc_dstrx3_get_hk_telemetry':
.../sc_dstrx3.c:132:83: warning: implicit conversion from 'float'
to 'double' to match other operand of binary expression [-Wdouble-promotion]
132 | LOG_DBG("VOLTAGE : %f [v] (raw: %d)", ((float)hk->voltage / 256) * 2.5,
| ^
The goal is to perform all calculations under float. Then, for this
particular case, convert it to double because the value is passed to a
variadic function, z_log_minimal_printk().
Related resources:
- https://en.cppreference.com/w/c/language/conversion
- Link to PR: zephyrproject-rtos/zephyr#57154
Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
In C, a literal value like "2.5" is considered a double. When you
multiply a float by a double, implicit promotion occurs, performing
the calculation in 64 bits. This unnecessarily consumes CPU power on
MCUs.
[118/166] Building C object .../sc_dstrx3.c.obj
In file included from .../logging/log.h:11,
from .../sc_dstrx3.c:9:
.../sc_dstrx3.c: In function 'sc_dstrx3_get_hk_telemetry':
.../sc_dstrx3.c:132:83: warning: implicit conversion from 'float'
to 'double' to match other operand of binary expression [-Wdouble-promotion]
132 | LOG_DBG("VOLTAGE : %f [v] (raw: %d)", ((float)hk->voltage / 256) * 2.5,
| ^
The goal is to perform all calculations under float. Then, for this
particular case, convert it to double because the value is passed to a
variadic function, z_log_minimal_printk().
Related resources:
- https://en.cppreference.com/w/c/language/conversion
- Link to PR: zephyrproject-rtos/zephyr#57154
Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
In C, a literal value like "2.5" is considered a double. When you
multiply a float by a double, implicit promotion occurs, performing
the calculation in 64 bits. This unnecessarily consumes CPU power on
MCUs.
[118/166] Building C object .../sc_dstrx3.c.obj
In file included from .../logging/log.h:11,
from .../sc_dstrx3.c:9:
.../sc_dstrx3.c: In function 'sc_dstrx3_get_hk_telemetry':
.../sc_dstrx3.c:132:83: warning: implicit conversion from 'float'
to 'double' to match other operand of binary expression [-Wdouble-promotion]
132 | LOG_DBG("VOLTAGE : %f [v] (raw: %d)", ((float)hk->voltage / 256) * 2.5,
| ^
The goal is to perform all calculations under float. Then, for this
particular case, convert it to double because the value is passed to a
variadic function, z_log_minimal_printk().
Related resources:
- https://en.cppreference.com/w/c/language/conversion
- Link to PR: zephyrproject-rtos/zephyr#57154
Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
Too many times, code is pushed that includes floats that really becomes doubles and C implicit promotion rules will want to make floats into doubles very easily. As Zephyr primarily targets low-end processors that may not have a double precision floating point unit, enable this flag globally for now.
DEPENDS ON
and likely others to come as the CI finds it all
west.yml also needed to be updated to include the fixes above with the fixes merged in to their "Zephyr forks"