Fix manual_div_ceil clippy lints

[jannic]

No description provided.

[thejpster]

Gloria Estefan and Miami Sound Machine were wrong - it's rustfmt that's gunna get ya.

[jannic]

It was obviously naive to expect that cargo clippy --fix creates properly formatted code.

[thejpster]

I think this makes the code worse at opt-level 0.

https://rust.godbolt.org/z/cGdPhs1Kj

An add and divide the compiler can optimise because it can see the divide is by a multiple of two. Using div_ceil means there's a function call and that optimisation no longer applies.

At opt-level=1 it's not as bad, but still different:

https://rust.godbolt.org/z/ceb4xKz8z

Edit: I suppose it's trying to avoid integer overflow in the general case.

[ithinuel]

And the div_ceil also works for non-power of two.

IMHO, in functions like ep_allocate which is expected to only be used a few times at the start of the system. The loss in performance may not be too bad.

But in the UART function I think we can locally allow the manual impl because we may be on a critical path (eg changing baudrate between data frames).

[thejpster]

This is not true any more.

[thejpster]

This is not true any more.

[thejpster]

That's a good idea

[jannic]

Looking at the UART code as it seems to be more performance critical. If I take the whole function (and not only the div_ceil), the two variants compare different. In the optimized case, they are nearly identical:
https://rust.godbolt.org/z/hn5nxbE4b
The compiler can use the fact that an integer overflow can no longer happen due to the & 0x7F.

I'm not sure if we need to care about the opt-level=0 case as long as it's not extraordinarily slow. (Ie. you can still use it when debugging non-performance-critical code.)

[thejpster]

Ok, I'm convinced.

Does this mean the comments about division still hold true?

[jannic]

Ok, I'm convinced.

Does this mean the comments about division still hold true?

I'll have a closer look. It's worth to check the rp2350 version as well, after all it's a different target so the assembly output may be different.

But ignoring the generated code: Do you think the div_ceil version is more readable, or is it a misguided approach in the first place?

I think it's clearer, but only by a small margin. So if the generated code turns out to be worse, we should keep the old code. Perhaps with a comment that it's equivalent to div_ceil, but faster.

[thejpster]

I think it's the same, or perhaps slightly less clear. But then I'm a grizzled old programmer so I'm familiar with the idea of X + (N - 1) / N as an idiom. Perhaps for those less familiar, a function that has nice documentation is better.