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

core/macros: avoid using SIGNOF for DIV_ROUND #18853

Closed
wants to merge 1 commit into from

Conversation

kfessel
Copy link
Contributor

@kfessel kfessel commented Nov 7, 2022

using SIGNOF triggers a compiler error see #18849

Contribution description

this removes SIGNOF from DIV_ROUND and DIV_ROUND_UP macro

the default implementation (_Generic will not use it) of SIGNOF would yield wrong values for values > ULLONG_MAX/2

Testing procedure

run the tests

Issues/PRs references

#18849

@kfessel kfessel requested a review from kaspar030 as a code owner November 7, 2022 12:48
@github-actions github-actions bot added the Area: core Area: RIOT kernel. Handle PRs marked with this with care! label Nov 7, 2022
@kfessel kfessel requested review from benpicco and maribu November 7, 2022 12:54
@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 7, 2022
@benpicco
Copy link
Contributor

benpicco commented Nov 7, 2022

What I don't like about this is that it introduces an additional division for those cases where the code is not compile-time const.

@riot-ci
Copy link

riot-ci commented Nov 7, 2022

Murdock results

✔️ PASSED

7411d61 core/macros: avoid using SIGNOF for DIV_ROUND

Success Failures Total Runtime
2000 0 2000 06m:22s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@kfessel
Copy link
Contributor Author

kfessel commented Nov 7, 2022

in many cases "div" and "mod" are calculated by the same call (if a buildin div is used that the buildin divmod is called once starting if -O2

and if the signof part is required the old variant is also not compact

https://godbolt.org/z/cojafofn4

e.g.: compiler option

-mthumb -march=armv7-m -O2 << hardware div
-mthumb -O2 << no hardware div

DIV_ROUND1(a, b) is this PR
DIV_ROUND2(a, b) is this master for signed types
DIV_ROUND3(a, b) is this master for unsigned types

@kfessel
Copy link
Contributor Author

kfessel commented Nov 9, 2022

closing this one since it provides a different and wrong result:

-15 / -5: master: 2 (wrong), this pr: 4 (wrong), correct: 3 see (#18856)
there are further examples where this pr and or master is wrong
see https://godbolt.org/z/h84nKzKv3

closing this in favour of #18856

@kfessel kfessel closed this Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants