cpu/cortexm_common: add inlined header only def for irq_% #13999

fjmolinas · 2020-05-01T07:30:43Z

Contribution description

irq_% are not inlined by the compiler which leads to it branching
to a function that actually implement a single machine instruction.

Inlining these functions makes the call more efficient as well as
saving some bytes in ROM.

This could probably be done for more arch, but I wanted to first do
it for a single arch first.

Testing procedure

All applications should still work
check footprint difference, BUILD_IN_DOCKER=1 BOARD=<board>make -C examples/gnrc_networking

iotlab-m3 cortexm3:

PR

  text    data     bss     dec     hex filename
  87536     184   19240  106960   1a1d0 /data/riotbuild/riotbase/examples/gnrc_networking/bin/iotlab-m3/gnrc_networking.elf

master

  text    data     bss     dec     hex filename
  87740     184   19240  107164   1a29c /data/riotbuild/riotbase/examples/gnrc_networking/bin/iotlab-m3/gnrc_networking.elf

samr21-xpro cortexm0:

PR

  text    data     bss     dec     hex filename
  91488     184   19240  110912   1b140 /data/riotbuild/riotbase/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

master

  text    data     bss     dec     hex filename
  91588     184   19240  111012   1b1a4 /data/riotbuild/riotbase/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

nrf52dk cortexm4:

PR

  text    data     bss     dec     hex filename
  162048     284   47616  209948   3341c /data/riotbuild/riotbase/examples/gnrc_networking/bin/nrf52dk/gnrc_networking.elf

master

  text    data     bss     dec     hex filename
  162248     284   47616  210148   334e4 /data/riotbuild/riotbase/examples/gnrc_networking/bin/nrf52dk/gnrc_networking.elf

Issues/PRs references

Came up a while ago in #11919

maribu · 2020-05-01T07:45:14Z

This is something I have wanted to see for a long time :-)

It is good to do this one arch at a time. I would also like to see the return value to be lets call it uword_t or irq_state_t, which is 32 bit wide on a 32 bit system, 16 bit on a 16 bit system, and 8 bit wide on a 8 bit platform.

Every platform I am aware of guarantees to disable and restore IRQs in one CPU cycle. As at most a word can be read/written in a cycle, this gives us the word as upper limit for the IRQ state.

The AVR platform would greatly profit if it would be both inlined and word sized, as then the call irq_disable() would translate to one instruction.

(But changing the return type is something that deserves its own PR. Especially as a lot of callers have to be updated.)

maribu

Looks good codewise. I have one suggestion inline. (And maybe you could also sneak in a bugfix? See inline.)

core/include/irq.h

cpu/cortexm_common/include/irq_arch.h

fjmolinas · 2020-05-04T07:01:01Z

Rebased.

fjmolinas · 2020-05-04T07:02:20Z

Will trigger murdock once to see If I missed an arch.

maribu · 2020-05-04T07:29:04Z

Code looks good to me. I think you can squash. (Also: The missing #ifdef __cplusplus in cpu/esp_common/include/cpu_conf_common.h can be just squashed in.)

fjmolinas · 2020-05-04T07:35:26Z

Code looks good to me. I think you can squash. (Also: The missing #ifdef __cplusplus in cpu/esp_common/include/cpu_conf_common.h can be just squashed in.)

Squashed. I triggered tests as well for good measure.

fjmolinas · 2020-05-04T07:39:50Z

Missed a cpu_conf. Will make sure I'm not missing more before pushing again.

maribu · 2020-05-11T08:41:19Z

I wouldn't mind a second review/ACK here, as this PR has a high impact.

fjmolinas · 2020-05-11T08:44:24Z

I wouldn't mind a second review/ACK here, as this PR has a high impact.

Yep I agree!

fjmolinas · 2020-05-11T08:44:52Z

@kaspar030 or @bergzand might be able to take a look at this one as well?

kaspar030

Some late comments.

core/include/irq.h

maribu

My ACK is still valid

irq_% are not inlined by the compiler which leads to it branching to a function that actually implement a single machine instruction. Inlining these functions makes the call more efficient as well as saving some bytes in ROM.

__set_PRIMASK(state) had been directly inlined to avoid a hardfault that occured when branching after waking up from sleep with DBG_STANDBY, DBG_STOP or DBG_SLEEP set in DBG_CR. The hardfault occured when returning from the branch to irq_restore, since the function is now inlined the branch does not happen either. Refer to RIOT-OS#14015 for more details.

fjmolinas · 2020-05-12T15:39:59Z

Build failed because there was no space left on worker https://ci.riot-os.org/RIOT-OS/RIOT/13999/b5e4224a6fe2de4aca35d90af5e76d5b6a5bf519/output/compile/tests/mtd_mapper/stk3600:gnu.txt

fjmolinas · 2020-05-12T16:14:49Z

All green again @kaspar030 !

kaspar030

ACK.

fjmolinas · 2020-05-12T20:01:57Z

Thanks for the review @kaspar030 @maribu!

maribu · 2020-05-13T07:19:58Z

Thanks for the PR. This is huge improvement (at least without LTO) :-)

fjmolinas added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: core Area: RIOT kernel. Handle PRs marked with this with care! labels May 1, 2020

fjmolinas requested review from DipSwitch, haukepetersen, kaspar030 and thomaseichinger as code owners May 1, 2020 07:30

fjmolinas requested a review from maribu May 1, 2020 07:30

maribu reviewed May 1, 2020

View reviewed changes

core/include/irq.h Outdated Show resolved Hide resolved

cpu/cortexm_common/include/irq_arch.h Outdated Show resolved Hide resolved

fjmolinas requested review from aabadie, benpicco, gschorcht, kYc0o and roberthartung as code owners May 4, 2020 06:59

fjmolinas force-pushed the pr_cortexm_inline_irq branch from c9f0db7 to f3dfb74 Compare May 4, 2020 07:01

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 4, 2020

fjmolinas force-pushed the pr_cortexm_inline_irq branch from 95201ba to 7a94b18 Compare May 4, 2020 07:34

fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 4, 2020

maribu mentioned this pull request May 4, 2020

tests: Add test for irq_disable() / irq_restore() #14011

Merged

fjmolinas force-pushed the pr_cortexm_inline_irq branch from 7a94b18 to 3ea0b4f Compare May 4, 2020 08:17

fjmolinas added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label May 11, 2020

kaspar030 added this to Backlog / Proposals in RIOT Sprint Day via automation May 12, 2020

kaspar030 self-assigned this May 12, 2020

kaspar030 moved this from Backlog / Proposals to Under Review in RIOT Sprint Day May 12, 2020

kaspar030 requested changes May 12, 2020

View reviewed changes

core/include/irq.h Outdated Show resolved Hide resolved

maribu approved these changes May 12, 2020

View reviewed changes

fjmolinas added 3 commits May 12, 2020 16:37

cpu/cortexm_common: add inlined header only def for irq_%

cb5cbe7

irq_% are not inlined by the compiler which leads to it branching to a function that actually implement a single machine instruction. Inlining these functions makes the call more efficient as well as saving some bytes in ROM.

cpu/cortexm_common/irq_arch: fix irq_enable return type

4ad3164

fjmolinas force-pushed the pr_cortexm_inline_irq branch from 1bec2be to b5e4224 Compare May 12, 2020 14:37

fjmolinas 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 May 12, 2020

kaspar030 approved these changes May 12, 2020

View reviewed changes

RIOT Sprint Day automation moved this from Under Review to Waiting For Murdock May 12, 2020

kaspar030 merged commit da2230d into RIOT-OS:master May 12, 2020

RIOT Sprint Day automation moved this from Waiting For Murdock to Done May 12, 2020

fjmolinas deleted the pr_cortexm_inline_irq branch May 12, 2020 20:02

maribu mentioned this pull request May 15, 2020

cpu/atmega_common: Update to inlineable IRQ API #14085

Merged

benpicco mentioned this pull request May 15, 2020

cpu/arm7_common: Update to inlineable IRQ API #14088

Merged

miri64 added this to the Release 2020.07 milestone Jun 24, 2020

maribu mentioned this pull request Jun 25, 2020

[tracking] Transition to inline-able IRQ API #14356

Closed

7 tasks

gschorcht mentioned this pull request Jun 25, 2020

gnrc/icmpv6: conditional definition of the MIN macro #14358

Merged

maribu mentioned this pull request Jun 25, 2020

cpu/msp430_common: Update to inline-able IRQ API #14362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/cortexm_common: add inlined header only def for irq_% #13999

cpu/cortexm_common: add inlined header only def for irq_% #13999

fjmolinas commented May 1, 2020

maribu commented May 1, 2020 •

edited

maribu left a comment

fjmolinas commented May 4, 2020

fjmolinas commented May 4, 2020

maribu commented May 4, 2020

fjmolinas commented May 4, 2020

fjmolinas commented May 4, 2020 •

edited

maribu commented May 11, 2020

fjmolinas commented May 11, 2020

fjmolinas commented May 11, 2020

kaspar030 left a comment

maribu left a comment

fjmolinas commented May 12, 2020

fjmolinas commented May 12, 2020

kaspar030 left a comment

fjmolinas commented May 12, 2020

maribu commented May 13, 2020

cpu/cortexm_common: add inlined header only def for irq_% #13999

cpu/cortexm_common: add inlined header only def for irq_% #13999

Conversation

fjmolinas commented May 1, 2020

Contribution description

Testing procedure

Issues/PRs references

maribu commented May 1, 2020 • edited

maribu left a comment

Choose a reason for hiding this comment

fjmolinas commented May 4, 2020

fjmolinas commented May 4, 2020

maribu commented May 4, 2020

fjmolinas commented May 4, 2020

fjmolinas commented May 4, 2020 • edited

maribu commented May 11, 2020

fjmolinas commented May 11, 2020

fjmolinas commented May 11, 2020

kaspar030 left a comment

Choose a reason for hiding this comment

maribu left a comment

Choose a reason for hiding this comment

fjmolinas commented May 12, 2020

fjmolinas commented May 12, 2020

kaspar030 left a comment

Choose a reason for hiding this comment

fjmolinas commented May 12, 2020

maribu commented May 13, 2020

maribu commented May 1, 2020 •

edited

fjmolinas commented May 4, 2020 •

edited