Prevent unnecessary bounds check in SCB::{get_priority, set_priority}

[qwerty19106]

SystemHandler.index() gives true index for SCB.shpr array.
But now it produce unnecessary bounds check. For example, SCB::set_priority:

48: sym.cortex_m::peripheral::scb::__impl_cortex_m::peripheral::SCB_::set_priority::h622cd21bcc3321be ();
0x08000376      push    {r7, lr}
0x08000378      mov     r7, sp
0x0800037a      bl      cortex_m::peripheral::scb::SystemHandler::index::hdaf1a31aa5a6a8c5 ; sym.cortex_m::peripheral::scb::SystemHandler::index::hdaf1a31aa5a6a8c5
0x0800037e      subs    r0, 4
0x08000380      uxtb    r0, r0
0x08000382      cmp     r0, 0xb    ; 11
0x08000384      itttt   ls
0x08000386      movw    r1, 0xed18
0x0800038a      movt    r1, 0xe000
0x0800038e      movs    r2, 0xff
0x08000390      strb    r2, [r0, r1]
0x08000392      it      ls
0x08000394      pop     {r7, pc}
0x08000396      movw    r2, 0x1dc0
0x0800039a      movs    r1, 0xc
0x0800039c      movt    r2, 0x800
0x080003a0      bl      core::panicking::panic_bounds_check::h5181f47ae17a04a5 ; sym.core::panicking::panic_bounds_check::h5181f47ae17a04a5
0x080003a4      trap

This PR fix it.

[rust-highfive]

r? @therealprof

(rust_highfive has picked a reviewer for you, use r? to override)

[therealprof]

The code change looks good to me, I'm not quite happy about the unsafety comments, though.

prevent unnecessary bounds check!

I don't think we need to provide a motivation here. The use of get_unchecked makes it quite clear that the motivation here is to get rid of bound checks.

Index of shpr array is true by SystemHandler design.

I don't know what true means in this context. Should rather say something along lines of "index is bounded to [x,y] by SystemHandler design".

Alternatively we could also throw in an assert to ensure that this invariant is never violated and point to that instead. Not sure how that would fare with code generation though.

[qwerty19106]

I tried adding assert!(index <16), but the compiler could not optimize it.

Is there a reason why we cannot change SystemHandler definition to:

pub enum SystemHandler {
    MemoryManagement = 4,
    ...

?

[therealprof]

Let's see how that goes.

[therealprof]

@qwerty19106 Can you check whether #203 does the trick on its own or whether we still need to change use get_unchecked, too, to achieve better code?

[qwerty19106]

On #203 the code just got a little easier, but panic_bounds_check is still present.

With temporary disabled #[inline] and armv7m target:

40: sym.cortex_m::peripheral::scb::__impl_cortex_m::peripheral::SCB_::set_priority::h7df537b11de4024c (int16_t arg2, int16_t arg3);
; arg int16_t arg2 @ r1
; arg int16_t arg3 @ r2
0x08000efc      push    {r7, lr}
0x08000efe      mov     r7, sp
0x08000f00      subs    r0, r1, 4  ; arg2
0x08000f02      uxtb    r0, r0
0x08000f04      cmp     r0, 0xb    ; 11
0x08000f06      itttt   ls
0x08000f08      movw    r1, 0xed18
0x08000f0c      movt    r1, 0xe000
0x08000f10      strb    r2, [r0, r1] ; arg3
0x08000f12      pop     {r7, pc}
0x08000f14      movw    r2, 0x2414
0x08000f18      movs    r1, 0xc
0x08000f1a      movt    r2, 0x800
0x08000f1e      bl      core::panicking::panic_bounds_check::h5181f47ae17a04a5 ; sym.core::panicking::panic_bounds_check::h5181f47ae17a04a5
0x08000f22      trap

I updated unsafe comments. Now it produce this code:

16: sym.cortex_m::peripheral::scb::__impl_cortex_m::peripheral::SCB_::set_priority::hd3dc9949907d960c (int16_t arg2, int16_t arg3);
; arg int16_t arg2 @ r1
; arg int16_t arg3 @ r2
0x08000e18      subs    r0, r1, 4  ; arg2
0x08000e1a      movw    r1, 0xed18
0x08000e1e      movt    r1, 0xe000
0x08000e22      uxtb    r0, r0
0x08000e24      strb    r2, [r0, r1] ; arg3
0x08000e26      bx      lr

I think one should to create issue in the Rust repository, since #203 should give enough information for compiler. I could do it if you don't mind.

[jonas-schievink]

I think one should to create issue in the Rust repository, since #203 should give enough information for compiler. I could do it if you don't mind.

Agreed, this looks like a missed optimization. I'll file an issue.

[jonas-schievink]

Looks like it's an old issue: rust-lang/rust#13926

[therealprof]

@jonas-schievink Do you have a strong opinion on whether we should make this change in the interim?

[therealprof]

LGTM, thanks.

[jonas-schievink]

I don't have anything against merging this, but the comments should mention the upstream issue.

[qwerty19106]

Added TODO comment about rust-lang/rust#13926

[therealprof]

Excellent, thanks you.

[therealprof]

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push