constify some checked arithmetics

[llogiq]

This makes the checked_{add, sub, mul, "neg", "shl", "shr"} operations const fns. Division is a bit more involved. I guess I could solve it with some trickery, but I'm not sure if it would negatively affect performance, so I stopped there.

[rust-highfive]

r? @TimNN

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

[tesuji]

r? @oli-obk

[oli-obk]

Oh no 🤣 this PR was bound to happen. I love it and hate it at the same time. I think llvm will clean this up nicely so it's not a perf problem (maybe in debug mode though) but it's still weird code that we'll surely revert once we get const if

cc @ecstatic-morse do you have a timeline on const branching?

I'd really prefer not to do this PR. cc @RalfJung

[ecstatic-morse]

See this comment, although perhaps the optimizer has gotten better?

Right now, I consider branching in consts blocked on #63812, but it could be implemented today if there's an urgent need. I can coordinate with @eddyb to drive #63812 forward.

edit:

It has not gotten better, although this tests abs, not checked arithmetic

[Centril]

Right now, I consider branching in consts blocked on #63812, but it could be implemented today if there's an urgent need. I can coordinate with @eddyb to drive #63812 forward.

Please take your time and absolutely do not rush anything even a little bit.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 257fb1e with merge 9c45d16...

[ecstatic-morse]

@Centril Does rustc-perf test the runtime performance of generated code? Or just the time it takes to compile that code? If it's the latter, I suspect that data won't be very useful here, since I doubt that rustc uses checked arithmetic in a hot loop anywhere. We would want to use something like (the artist formerly known as) lolbench on some numerical libraries written in pure rust.

[Centril]

compile time is being tested; I have no idea if checked arithmetic is being used often but it can't hurt to check I think.

[oli-obk]

Please take your time and absolutely do not rush anything even a little bit.

This very much. My question was just that, without any subtext. I also forgot that having the feature unstably won't help these functions on their way to stability.

I propose to close this since it seems to not get optimized as well as the code with branching

[ecstatic-morse]

Please take your time and absolutely do not rush anything even a little bit.

This very much. My question was just that, without any subtext. I also forgot that having the feature unstably won't help these functions on their way to stability.

I propose to close this since it seems to not get optimized as well as the code with branching

Just to clarify, should I or should I not rush? 😆

[llogiq]

@oli-obk I'll benchmark, but I'll take a while. First I need some sleep. 💤

[bors]

☀️ Try build successful - checks-azure
Build commit: 9c45d16 (9c45d164d2b4d914b5c53b0f3ddd09438fcd89b8)

[rust-timer]

Queued 9c45d16 with parent 2e72448, future comparison URL.

[RalfJung]

This is exactly what I expected when #63786 happened. ;)

Ultimately it is up to T-libs if they want to maintain such code.

I also forgot that having the feature unstably won't help these functions on their way to stability.

Why not, with the "use internal unstable" mechanism that we have these days?

[rust-timer]

Finished benchmarking try commit 9c45d16, comparison URL.

[tspiteri]

The performance does seem to suffer quite a bit, the moves from the indexing do not seem to be optimized out, and llvm-mca reports a regression in reciprocal throughput by a factor of 2.5, from 1.2 to 3.0: https://godbolt.org/z/lYIVmr, so to me this PR does not look like a clear win even ignoring the const-hacky loss in readability.

[tspiteri]

Also, I do not see the usefulness of this: these functions return Option, and a constant Option does not have an obvious use case, as the Option needs to be used in a match anyway so it defies the constness point until match can be used in constants, when the hack becomes unnecessary. For constants it is already possible to use the available overflowing_* const methods.

I don't mean that the methods should never be const, just that there is no adequate compensation for the present loss in performance with this implementation.

[RalfJung]

Also, I do not see the usefulness of this: these functions return Option

Oh, I didn't even realize this.
@llogiq give that Option AFAIK has no const fn methods and match cannot be used in const fn either, what is the point in making these functions const through hacks?

I don't mean that the methods should never be const

Once if works in const fn, we'll certainly make them const as there'll be no cost to that at that point. :)

[RalfJung]

Btw, with #68809 these functons are now const.