add avx512 pshufb #4764
add avx512 pshufb #4764
Conversation
|
Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two. |
| /// <https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm_shuffle_epi8> | ||
| /// <https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm256_shuffle_epi8> | ||
| /// <https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm512_shuffle_epi8> | ||
| fn pshufb<'tcx>( |
There was a problem hiding this comment.
Could you add an explanation of what the types here are? Are these all u8 vectors?
There was a problem hiding this comment.
It has currently been changed to:
/// Shuffles bytes from `left` using `right` as pattern.
///
/// `left` and `right` are both vectors of type `len` x i8. Only bits 0, 1, 2, 3 and 7 of each byte of
/// `right` matter; if bit 7 of each byte of `right` is set, the value of `dest` at the corresponding
/// byte will be set to 0.
///
/// Each 128-bit block is shuffled independently.
There was a problem hiding this comment.
It should also say what the other 4 bits do then. I guess it's something like this:
The first four bytes of right at index i indicate which of the left values from the same 16-element block is used for index i in dest.
There was a problem hiding this comment.
Now it's
/// Shuffles bytes from `left` using `right` as pattern. Each 16-byte block is shuffled independently.
///
/// `left` and `right` are both vectors of type `len` x i8.
///
/// If the highest bit of a byte in `right` is not set, the corresponding byte in `dest` is taken from
/// same 16-byte block of `left` at the position indicated by the lowest 4 bits of this byte in `right`.
/// If the highest bit of a byte in `right` is set, the corresponding byte in `dest` is set to `0`.
src/shims/x86/mod.rs
Outdated
|
|
||
| let res = if right & 0x80 == 0 { | ||
| // Shuffle each 128-bit (16-byte) block independently. | ||
| let j = u64::from(right % 16).strict_add(i & !15); |
There was a problem hiding this comment.
Is the i & !15 here the same as i / 16? If yes, think that would be more clear.
There was a problem hiding this comment.
No. It's i / 16 * 16.
There was a problem hiding this comment.
Ah, right.
| let j = u64::from(right % 16).strict_add(i & !15); | |
| let block_start = i & !15; // round down to previous multiple of 16 | |
| let j = block_start.strict_add((right % 16).into()); |
There was a problem hiding this comment.
Did you check this on real hardware?
There was a problem hiding this comment.
Yes. I've tested intrinsics-x86-avx512.rs on real hardware. I also ran miri on my real cases.
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
@rustbot ready |
rustbot
added
S-waiting-on-review
| let b = _mm512_set_epi8(-1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, | ||
| -1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, | ||
| -1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, | ||
| -1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1); |
There was a problem hiding this comment.
Using some indices other than 1 also seems like a good idea. In particular, please ensure the "wrap-around" is tested by also checking 127.
Why does index 1 read "14" for the first block? It seems to be indexing from the right...?
There was a problem hiding this comment.
Why does index 1 read "14" for the first block?
_mm512_set_epi8 sets the elements of a vector in reverse order. To set the elements in forward order, _mm512_setr_epi8 should be used.
There was a problem hiding this comment.
Oh wow... what an odd choice for an API.
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
Thanks! I have done some more tweaking to the comments. I'd appreciate if you could also submit a PR to stdarch to update the test there as well -- it's generally preferable to the tests to be in sync between the two projects. |
3 participants
Used to implement the _mm512_shuffle_epi8 intrinsic.