Add `checked_add` and `checked_sub`
kimikage opened this issue · 8 comments
As the first measure against #41, I want to add checked_add and checked_sub (and checked_neg, checked_abs) so that we can try CheckedArithmetic.
I think it is better to support checked_mul at the same time as or after the optimization of current multiplication implements. Adding checked_mul implies introducing unchecked mul, i.e. breaking changes.
BTW, if checked arithmetic becomes the default and the overflow-safe promotion rules are added in the future, I think that using the @fastmath macro to access the successor methods to the current unsafe arithmetic might be a good idea. However, I cannot compromise any more than that.
BTW, the integer types of Rust have saturating_add(), wrapping_sub() and so on, and their default operators check overflows.
The removal of @fastmath (changing to more specific methods) was proposed (cf. JuliaLang/julia#36246). I think adding add_fast and so on for FixedPoint seems to be a bad idea, now.
One option is to add the @saturating and @wrapping macros to CheckedArithmetic (or another new package). I think the new macros also solves the problem that @checked is too exhaustive.
Also, in the future, the default arithmetic may be changed to the checked arithmetic.
What's your plan for eliminating the performance hit?
Originally posted by @timholy in #209 (comment)
It is to give up.:sweat_smile: In any case, we haven't reached the stage of deciding that yet. PR #190 is just a small step.
Originally posted by @kimikage in #209 (comment)
However, I am interested in Cassette.jl. I think the injection is useful in end user codes. If checked arithmetic is needed only for development/debugging and not for the final code, I think that the default arithmetic can be the current wrapping arithmetic.
It might be also a good idea for CheckedArithmetic.jl to use Cassette.jl. However, in that case, it is better to separate the interface and implementation into separate packages. As you know, I have concerns about the accumulatortype. (cf. #146 (comment))
In the future (Julia v2.0?), I hope that handling wrapping/saturating/checked arithmetic switching should be supported by Base or Core. I would like to do the case study for that within FixedPointNumbers. If the case study goes well, it will be the motivation to support saturated arithmetic functions (and the intrinsic functions for LLVM) in Base as of Julia v1.x.
Cassette is cool technology but beware of the compile-time latency. Conceptually it's not very different from
img1checked, img2checked = reinterpret(N0f8Checked, img), reinterpret(N0f8Checked, img)
imgdiff = img1checked-img2checkedwhich would be a more "surgical" approach to checked-aware recompilation (though perhaps less easy to automate).
While catching issues during development is of course a good idea, don't forget the interactive convenience of visualizing img1 - img2, and the head-scratching that can result when you have wrapping or saturating arithmetic. Checked arithmetic is also quite annoying in such circumstances, though at least it might give you an inkling of what's to blame. float.(img1) - float.(img2) is an easy way to fix problems but not all users will think of it.
The original purpose of this issue was achieved by PR #190, but I reopened this just for discussion.
Oh, I think you could leave this closed if you want. As you say, you fixed it! 🎉 Having checked arithmetic is great, I'm just pointing out that it doesn't solve all the problems that I think we might want to address.
It may be better to open the topic in the Discourse or Zulip. 😄