Repository navigation
Add widening integer ops - #415
Conversation
|
This can be seen in action in https://github.com/Shnatsel/jpeg-encoder/tree/fearless-simd |
LaurenzV
left a comment
LaurenzV
left a comment
There was a problem hiding this comment.
API looks good to me! Just hope there are no hidden traps regarding breaking changes. 😄
| ], | ||
| ), | ||
| ]; | ||
| for (a, b) in cases { |
There was a problem hiding this comment.
Up to you, I personally think it would be better if the test oracle consists of hardcoded numbers instead of just "replaying" with scalar Rust and then asserting the results are the same. Same for the widening add/mul tests.
But fine from my side if you prefer it this way!
There was a problem hiding this comment.
I generally follow the style you describe, but in this particular case the operation is so arcane that it would be difficult for me to debug if something broke with just hardcoded expected values. I would question whether the expected data was generated correctly, how it was generated, etc. I think in this particular case a scalar reference is easier to follow.
| /// A native-width SIMD vector of [`u8`]s. | ||
| type u8s: SimdInt<Self, Element = u8, Block = u8x16<Self>, Mask = Self::mask8s, ByteVector = Self::u8s> | ||
| + SimdWiden<Self, Widened = Self::u16s>; | ||
| + SimdIntWiden<Self, Widened = Self::u16s>; |
There was a problem hiding this comment.
This is not a breaking change only because the trait is sealed, right?
There was a problem hiding this comment.
Correct.
I also hope that cargo-semver-checks would scream at us if this weren't the case, but I haven't verified.
There was a problem hiding this comment.
In theory if someone imported everything from this module, their code could stop compiling if they have other widening_mul etc. methods in-scope, right? 🤔 Not sure if that's considered a breaking change though.
There was a problem hiding this comment.
Concrete imports take precedence over wildcard imports. So it would still work unless they used wildcard imports for both.
Cargo explicitly calls this out as a minor change, despite the potential glob collisions: https://doc.rust-lang.org/cargo/reference/semver.html#item-new
|
Related to #382 The pairwise single vector version seems more fundamental than elementwise widening addition, though, as far as I can tell. |
3 participants
Adds a new
SimdIntWidentrait for widening ops on integers, requiring bothSimdIntandSimdWiden.Adds three ops on that trait:
jpeg-encodercrate to Fearless SIMD.Probably best reviewed commit by commit.