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

Simplify and reduce code duplication in arithmetic.rs #1160

Closed
jhorstmann opened this issue Jan 11, 2022 · 0 comments · Fixed by #1161
Closed

Simplify and reduce code duplication in arithmetic.rs #1160

jhorstmann opened this issue Jan 11, 2022 · 0 comments · Fixed by #1161
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog

Comments

@jhorstmann
Copy link
Contributor

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
I noticed that there is some potential for simplification or reduction of code in arithmetic.rs, that should make it easier to add more functionality.

Describe the solution you'd like

  • unify the simd logic for modulo / division, the complex part of checking for a valid zero is duplicated in both, instead that could be extracted into another helper with the actual logic in a closure
  • the ArrowSignedNumericType seems to be only used for the simd negation operation, instead that operation could substract the given value from 0. X86_64 does not even seem to have a separate negation instruction, so the compiler would have already translated that into a substraction
  • many of the generic bounds for the arithmetic functions are broader than needed, probably due to copy+pase. For example multiply only needs a bound of T::Native: Mul<Output = T::Native>

Describe alternatives you've considered

We might also investigate how much the simd kernels actually improve performance, in many cases the compiler should be able to auto-vectorize the scalar implementations and produce the same code.

For floating point types we could also implement an unchecked division kernel, as division by zero is well-defined for those types.

@jhorstmann jhorstmann added the enhancement Any new improvement worthy of a entry in the changelog label Jan 11, 2022
@alamb alamb added enhancement Any new improvement worthy of a entry in the changelog arrow Changes to the arrow crate and removed enhancement Any new improvement worthy of a entry in the changelog labels Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
2 participants