sys/fmt: Small optimizations #7366
Conversation
jnohlgard
added
Type: enhancement
|
I actually can't confirm that: I used the following compilers: (did not test on MIPS) |
|
@miri64 Which test application are those numbers taken from? |
|
(With "the offending lines" I meant just the "ifneq (,$(filter tests-cpp_%, $(UNIT_TESTS)))" block ;-)) |
sys/fmt/fmt.c
Outdated
| @@ -183,12 +183,12 @@ size_t fmt_u16_dec(char *out, uint16_t val) | |||
|
|
|||
| size_t fmt_s32_dec(char *out, int32_t val) | |||
| { | |||
| int negative = (val < 0); | |||
| size_t negative = (val < 0 ? 1 : 0); | |||
There was a problem hiding this comment.
why not unsigned, as used in other places? Even (unsigned) char would do, but will likely not be more optimal in size, anyway.
There was a problem hiding this comment.
and why ternary here - to be more explicit? If so I'd rather write it as `(val < 0) ? 1 :0;
There was a problem hiding this comment.
yeah, to be more readable, the parentheses should be around val<0
sys/fmt/fmt.c
Outdated
| @@ -183,12 +183,12 @@ size_t fmt_u16_dec(char *out, uint16_t val) | |||
|
|
|||
| size_t fmt_s32_dec(char *out, int32_t val) | |||
| { | |||
| int negative = (val < 0); | |||
| size_t negative = (val < 0 ? 1 : 0); | |||
There was a problem hiding this comment.
yeah, to be more readable, the parentheses should be around val<0
sys/fmt/fmt.c
Outdated
| @@ -262,11 +262,11 @@ size_t fmt_float(char *out, float f, unsigned precision) | |||
| { | |||
| assert (precision <= 7); | |||
|
|
|||
| unsigned negative = (f < 0); | |||
| unsigned negative = ((f < 0) ? 1 : 0); | |||
There was a problem hiding this comment.
IMO the outer parens can go. Anyhow, remove them here or add above, please!
jnohlgard
force-pushed
the
pr/fmt-optimizations
branch
from
6386700 to
f6d6250
Compare
smlng
added
the
CI: needs squashing
did you compile with "RIOT_VERSION_OVERRIDE=test" in both master and this PR's branch? |
|
I see 16byte size reduction when compiling "unittests/tests-fmt" for samr21-xpro and mulle, using "gcc version 7.1.0 (Arch Repository)". |
sys/fmt/fmt.c
Outdated
| @@ -194,12 +194,12 @@ size_t fmt_u16_dec(char *out, uint16_t val) | |||
|
|
|||
| size_t fmt_s32_dec(char *out, int32_t val) | |||
| { | |||
| int negative = (val < 0); | |||
| unsigned negative = (val < 0) ? 1 : 0; | |||
There was a problem hiding this comment.
Looking at the side-by-side diff, does appending ? 1 : 0 really increase readability?
There was a problem hiding this comment.
on second look, maybe not.
There was a problem hiding this comment.
IMO (val < 0) is perfectly readable and to the point.
There was a problem hiding this comment.
@gebart what do you think?
There was a problem hiding this comment.
@gebart what do you think?
Just saw that you already replied. IMO if it is unclear which is better to read, let's lower the amount of characters that our poor brains need to parse.
|
I see a size reduction, too for arduino-uno of 26B for text and 8B for data. But on native macOS I get an error for the now unused |
What compiler version? |
|
avr-gcc (GCC) 7.1.0 |
kaspar030
added
the
CI: ready for build
Let's see what the CI compilers say. |
|
also: need to remove (or comment out) unused |
yes! |
|
Hm, in many cases, this PR increases codesize: https://gist.github.com/kaspar030/50d9852f9aaec1c5de72fcac23b102d9 This compares sizes.json from the PR's CI build with a manually built sizes.json from the PR's base branch at build time. I assume that any application using the division helper elsewhere gains weight. |
|
I made some tests, and removed this saves 10++ bytes here and there, but will likely consume some CPU cycle. So which is to optimize: binary size or cpu cycles? More cpu cycles may mean more energy consumption, right? |
|
how do we proceed with this one? IMHO if binary size is critical don't use FMT, on the other hand with #7499 added on top of this it reduces overall size of the FMT module again. |
|
@kaspar030 how do we move this (and related PRs #7499, and #7263) forward? |
Well, fmt is supposed to be a lightest-weight alternative to *printf...
Let's merge this. It definately looks cleaner. |
sys/fmt/fmt.c
Outdated
| @@ -194,12 +194,12 @@ size_t fmt_u16_dec(char *out, uint16_t val) | |||
|
|
|||
| size_t fmt_s32_dec(char *out, int32_t val) | |||
| { | |||
| int negative = (val < 0); | |||
| unsigned negative = (val < 0) ? 1 : 0; | |||
There was a problem hiding this comment.
@gebart what do you think?
sure, what I meant was that if you really care about size (or have a production release) you might discard output completely, hence no |
|
I agree with @smlng, the form |
Please squash directly! |
jnohlgard
force-pushed
the
pr/fmt-optimizations
branch
from
1ef08e2 to
7a20521
Compare
jnohlgard
force-pushed
the
pr/fmt-optimizations
branch
from
7a20521 to
1e43bd4
Compare
|
@smlng fixed, squashed |
kaspar030
added
CI: ready for build
This PR saves a few bytes on Cortex-M0 and reduces the number of calls to the long division helper functions (
__aeabi_uidiv) infmt_dec_u64, by using multiplication instead of division.Verified by manual inspection of compiler generated assembly code.