Fix in erroneous implementation of _mm256_bsrli_epi128 #1823
Conversation
Fixing the issue mentioned in issue rust-lang#1822 of rust-lang/stdarch.
satiscugcat
changed the title
crates/core_arch/src/x86/avx2.rs
Outdated
| @@ -191,7 +191,7 @@ pub fn _mm256_alignr_epi8<const IMM8: i32>(a: __m256i, b: __m256i) -> __m256i { | |||
| return transmute(a); | |||
| } | |||
|
|
|||
| let r: i8x32 = match IMM8 % 16 { | |||
| let r: i8x32 = match IMM8 { | |||
There was a problem hiding this comment.
You have to also add a value to the default arm. It is currently unreachable_unchecked(), which makes IMM8 values > 15 UB.
Edit: Maybe you should change the IMM8 == 16 if above to be IMM8 >= 16? Also there is handling for IMM8 > 16 above already: (_mm256_setzero_si256(), a) which would become unreachable in that case.
There was a problem hiding this comment.
Oh right, I was mistaken. I think specifically values between 17-31 (inclusive) are UB without the %, which makes it not redundant. My mistake!
|
Did you try reverifying _mm256_alignr_epi8 with whatever tool you used to find the _mm256_bsrli_epi128 bug? And if so how did the |
The _mm256_bsrli_epi128 bug was found manually upon noticing a discrepancy in the intel specifications and the rust implementations in that case, and then testing it as it is tested in issue #1822 . Thus the other bug slipped past me too. My bad! |
satiscugcat
changed the title
|
Could you check pls if this bug is also there for |
Sure! We'll work on testing the variants from tomorrow. |
|
just checked, seems like the same bug is there for Actually, would you mind changing the impl of the |
I've made the appropriate changes. |
…fixing a bug in the process.
This fixes the error mentioned in issue #1822