Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add saturating integer math #7

Merged
merged 25 commits into from
May 23, 2024
Merged

Add saturating integer math #7

merged 25 commits into from
May 23, 2024

Conversation

BioTurboNick
Copy link
Collaborator

No description provided.

@BioTurboNick
Copy link
Collaborator Author

@kimikage I know you were interested in this kind of extension, so I tried it out. Feel free to take a look of you have a minute.

@kimikage
Copy link

kimikage commented May 8, 2024

You really get your work done fast!

I have not been able to track down the details yet, but let me discuss the module configuration.
As checked_*s are in Base.checked, I would like to group the series of functions into submodules.
cf.JuliaMath/FixedPointNumbers.jl#292

I envision the following configuration:

  • OverflowContexts
    • Arithmetic (or something)
      • Unchecked ("don't-care" option)
      • Wrapping
      • Saturating
      • Checked

The submodules may be a premature optimization.
However, the practical side is that they make it easier to control function exposures (by means of import or using).
Of course, this is a matter of where the functions belong, not where they are implemented.

@BioTurboNick
Copy link
Collaborator Author

Thanks! Want to make sure I understand your suggestions.

Wrapping is just what Julia does now, not something else right?

I'm curious if you're aware of a reason the checked methods are in a Base submodule? It always seemed odd to me. But might make sense to echo that.

Why the Arithmetic intermediate?

@kimikage
Copy link

kimikage commented May 9, 2024

Wrapping is just what Julia does now, not something else right?

That is partially true.
The default integer arithmetic in julia behaves as checked with respect to division. That is the main reason why we need wrapping_*.
Another reason is that for some types, it may be better not to enforce wrapping, mainly for performance reasons. For example, for an unsigned type, one might want to default to a behavior such as zero-clamping for negative numbers with a short-circuit but not saturating for the upper bound.
It is also envisioned that in the future the default behavior will be configurable by Preferences on a per-package basis, such as FixedPointNumbers. In such cases, a clear distinction must be made between + and wrapping_add.

@kimikage
Copy link

kimikage commented May 9, 2024

I'm curious if you're aware of a reason the checked methods are in a Base submodule?

I don't know if this answers your question, but I think it means that Checked is actually needed in Base and stdlibs, and yet not as commonly needed as the default unchecked.
And just as you questioned the need for Wrapping, I don't see the need for wrapping_* to be exposed to the end user.

@kimikage
Copy link

kimikage commented May 9, 2024

Why the Arithmetic intermediate?

There are three major reasons.

  • I want to expose all types of arithmetic (excluding the macros) in using at once.
  • The names such as Checked are not descriptive enough by themselves. This is the discomfort I feel with Base.Checked.
  • More types of arithmetic could be added. Widen is a favorite of @timholy, I think.😄

If your question is why Arithmetic is not at the top, it would be fine if it were the top i.e. the package with a good name.
However, I have an awkward feeling about CheckedArithmeticCore.🙉

WRT Widen, it is not friendly with Julia's type inference, as you know.
Also, there are few options to choose from for standard Integers. Once you double the width, you are unlikely to double it again. I think Widen may be an effective strategy for types which spread in 1-bit or 8-bit increments.
So, I am in favor of leaving such possibilities open, but I do not think Widen should be a priority.

@BioTurboNick
Copy link
Collaborator Author

Thanks! That's helpful and suggests a roadmap.

I see your point about div. It goes to the intrinsic function checked_sdiv_int instead of sdiv_int.

Hmm. Apparently Base.checked_sdiv_int(typemin(Int), -1) is the one value vulnerable to overflow, but it emits a DivideError instead of an OverflowError. But Base.sdiv_int(typemin(Int), -1) doesn't overflow, it crashes Julia with a Win32 exception, EXCEPTION_INT_OVERFLOW. On Linux it's a DivideError for both functions.

@BioTurboNick
Copy link
Collaborator Author

So I think the underlying issue is that in LLVM, division overflows are deemed undefined behavior, unlike addition, subtraction, and multiplication.

https://llvm.org/docs/LangRef.html#add-instruction
https://llvm.org/docs/LangRef.html#sdiv-instruction

@BioTurboNick
Copy link
Collaborator Author

In the spirit of it being undefined behavior... wrapping sdiv_int in a function doesn't throw the errors above.

@kimikage
Copy link

kimikage commented May 10, 2024

For integers with small bitwidth, using floating-point division might not be such a bad choice, given the handling of RoundingMode as well (with SIMD).
(Speed may not be a important factor for now, since the current division can throw error. Performance tuning can be done later.)

@BioTurboNick
Copy link
Collaborator Author

After some thought, I think that the native default operators, other than for division, should always be considered unchecked. It's the most performant and it matches the defaults of other languages, and LLVM itself.

When the default varies from this, it's because there's a very good reason - undefined behavior would occur (integer division), or the type's existence is intended to prevent it (e.g. SaferIntegers).

So that gives us:

  • an unchecked (wrapping) default that's overridable;
  • checked_ methods that can be opted into; if not provided, uses default
  • and with this PR, saturating_ methods that can be opted into; if not provided, uses default

I'm wondering if we might even not need the submodules, might be a way to do all this without exporting anything... But if not, having the modules does make sense to echo Base.

So what to do about division operations?

E.g. C# doesn't turn off throwing exceptions in their unchecked context, so there's precedent for treating division overflows as distinct.

I still see some utility in making unchecked integer division methods available and switchable, but perhaps it just needs to be separated in some way. Maybe we can have an option passed to the macro to opt-in to unchecked division methods? I'll open an issue to explore that.

@BioTurboNick
Copy link
Collaborator Author

BioTurboNick commented May 13, 2024

I think I'm coming around to some of your initial proposal of having wrapping, checked, and saturating modes, and a default/unchecked mode.

And I'm also thinking we probably shouldn't have explicit wrapping/saturating/checked methods falling back to the default methods for Number types, because that could lead to silent mistakes.

If e.g. SaferIntegers or some other type ends up in an explicit wrapping or saturating context and doesn't have those specific methods, something is probably wrong and that needs to be fixed.

For Julia's built-in types, wrapping_ would just map to unchecked_ (pending some more thinking about #9 )

@BioTurboNick BioTurboNick merged commit 356e5d9 into main May 23, 2024
12 checks passed
BioTurboNick added a commit that referenced this pull request May 23, 2024
* Add saturating array methods and broadcast tests

* Bonus - add saturating test for SaferIntegers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants