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.

Sign up for GitHub

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

Jump to bottom

core/compiler_hints: add likely() / unlikely() hints #19156

Merged
merged 3 commits into from
Jan 19, 2023
Merged

core/compiler_hints: add likely() / unlikely() hints #19156

merged 3 commits into from
Jan 19, 2023

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jan 16, 2023 •
edited
Loading

Contribution description

The compiler can optimize the code better if it knows whether a condition is likely taken / not taken.

This is usually something the developer should not bother with, but for things like assert() where the condition should always be true, we can tell the compiler to optimize that path.

Testing procedure

No logic changes, CI will ensure that all our compilers know about the builtin.

Issues/PRs references

#19155 (comment)

@benpicco benpicco requested a review from kaspar030 as a code owner January 16, 2023 14:15
@benpicco benpicco requested a review from kfessel January 16, 2023 14:15
@github-actions github-actions bot added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: sys Area: System labels Jan 16, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 16, 2023
@benpicco benpicco requested a review from maribu January 16, 2023 14:16
maribu
maribu approved these changes Jan 16, 2023
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

lgtm and for assert() it definitely is worth the additional maintenance overhead.

See two inline nitpicks

core/lib/include/compiler_hints.h Outdated Show resolved Hide resolved
core/lib/include/compiler_hints.h Outdated Show resolved Hide resolved
@kfessel
Copy link
Contributor

kfessel commented Jan 16, 2023

likely and unlikely will not do the same as changin the optimization level since doing that kind of reordering might enlarge the build result (but still be faster since the hotpath is shorter)

godbolt

@kfessel
Copy link
Contributor

kfessel commented Jan 16, 2023

I think likely does not provide extra information to the compiler since the __attribute__((noreturn)) already makes clang behave as if that case is unlikely.
clang does the reordering if the condition is marked likely or the panic() is marked NORETURN
gcc does make the reodering with one of the makers (noreturn or likely) but not if Os is used only if O3/1/2

@riot-ci
Copy link

riot-ci commented Jan 16, 2023 •
edited
Loading

Murdock results

✔️ PASSED

d59819e sys/test_utils: mark expect() condition as likely true

Success Failures Total Runtime
6785 0 6785 15m:58s

Artifacts

@benpicco
Copy link
Contributor Author

bors merge

kfessel
kfessel requested changes Jan 16, 2023
View reviewed changes
Copy link
Contributor

@kfessel kfessel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

since assert.h implements a standard header it should not provide defines that are not part of that standard header

http://www.open-std.org/jtc1/sc22/wg14/www/C99RationaleV5.10.pdf :7.1.3 p 112 (pdf:119) "...the Standard assures that macro and typedef names are reserved only if the associated header is explicitly included"

#17574 (comment)

core/lib/include/assert.h Outdated Show resolved Hide resolved
bors bot added a commit that referenced this pull request Jan 16, 2023
@bors @jia200x @benpicco
Merge #18472 #18863 #19156
bcaeb9d 
18472: drivers/mrf24j40: add support for IEEE 802.15.4 Radio HAL r=benpicco a=jia200x



18863: boards/esp32s2-mini: add definition for ESP32 S2 Mini r=benpicco a=benpicco



19156: core/compiler_hints: add likely() / unlikely() hints r=benpicco a=benpicco



Co-authored-by: Jose Alamos <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jan 16, 2023

Build failed (retrying...):

@kfessel
Copy link
Contributor

kfessel commented Jan 16, 2023

bors cancel

@bors
Copy link
Contributor

bors bot commented Jan 16, 2023

Canceled.

@benpicco benpicco force-pushed the likely branch 3 times, most recently from 2a64380 to e75c702 Compare January 17, 2023 09:26
@benpicco benpicco added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: arduino API Area: Arduino wrapper API and removed Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: arduino API Area: Arduino wrapper API labels Jan 17, 2023
@benpicco
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 17, 2023

👎 Rejected by PR status

maribu
maribu approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 17, 2023

👎 Rejected by PR status

@maribu
Copy link
Member

maribu commented Jan 17, 2023

bors retry

@maribu
Copy link
Member

maribu commented Jan 17, 2023

bors cancel
bors merge

@bors
Copy link
Contributor

bors bot commented Jan 17, 2023

👎 Rejected by PR status

@maribu
Copy link
Member

maribu commented Jan 17, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 17, 2023

👎 Rejected by PR status

@maribu
Copy link
Member

maribu commented Jan 17, 2023

bors merge --force

@bors
Copy link
Contributor

bors bot commented Jan 17, 2023

👎 Rejected by PR status

@maribu
Copy link
Member

maribu commented Jan 17, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 17, 2023

👎 Rejected by PR status

@maribu
Copy link
Member

maribu commented Jan 17, 2023

bors, did you change your mind?
bors merge

@bors
Copy link
Contributor

bors bot commented Jan 17, 2023

👎 Rejected by PR status

@maribu
Copy link
Member

maribu commented Jan 17, 2023

bors retry

@bors
Copy link
Contributor

bors bot commented Jan 17, 2023

👎 Rejected by PR status

@benpicco
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 18, 2023

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 18, 2023
@benpicco
Copy link
Contributor Author

bors merge

@kfessel
Copy link
Contributor

kfessel commented Jan 19, 2023

bors r-

@kfessel
Copy link
Contributor

kfessel commented Jan 19, 2023

bors cancel

@kfessel
Copy link
Contributor

kfessel commented Jan 19, 2023

bors r+

@maribu
Copy link
Member

maribu commented Jan 19, 2023

bors Pending — Waiting in queue

🎉 So in 3h - 5h it will get merged :)

@bors
Copy link
Contributor

bors bot commented Jan 19, 2023

Build succeeded:

@bors bors bot merged commit 71f783f into RIOT-OS:master Jan 19, 2023
@benpicco benpicco deleted the likely branch January 19, 2023 18:03
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@kfessel kfessel kfessel approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

Assignees
No one assigned
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2023.04
Development

Successfully merging this pull request may close these issues.
5 participants
@benpicco @kfessel @riot-ci @maribu @MrKevinWeiss