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.

Sign up for GitHub

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

Jump to bottom

Fix x86 extract_epi{8,16} functions #868

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Fix x86 extract_epi{8,16} functions #868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions crates/core_arch/src/x86/avx2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3743,9 +3743,9 @@ pub unsafe fn _mm256_xor_si256(a: __m256i, b: __m256i) -> __m256i {
// This intrinsic has no corresponding instruction.
#[rustc_args_required_const(1)]
#[stable(feature = "simd_x86", since = "1.27.0")]
pub unsafe fn _mm256_extract_epi8(a: __m256i, imm8: i32) -> i8 {
pub unsafe fn _mm256_extract_epi8(a: __m256i, imm8: i32) -> i32 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the change below is technically a breaking change in the public API. However we are fixing the function here to match the definition. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we need a crater run at the very least for this. cc @rust-lang/libs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably going to break anyone that uses _mm256_extract_epi8, no?

It seems like this is probably an okay "wontfix" bug, since I think i8 is big enough to hold all possible bit patterns returned here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect most users of these intrinsics to cast the returned value immediately, which would hide the issue.

let imm8 = (imm8 & 31) as u32;
simd_extract(a.as_i8x32(), imm8)
simd_extract::<_, u8>(a.as_u8x32(), imm8) as i32
}

/// Extracts a 16-bit integer from `a`, selected with `imm8`. Returns a 32-bit
Expand All @@ -3759,9 +3759,9 @@ pub unsafe fn _mm256_extract_epi8(a: __m256i, imm8: i32) -> i8 {
// This intrinsic has no corresponding instruction.
#[rustc_args_required_const(1)]
#[stable(feature = "simd_x86", since = "1.27.0")]
pub unsafe fn _mm256_extract_epi16(a: __m256i, imm8: i32) -> i16 {
pub unsafe fn _mm256_extract_epi16(a: __m256i, imm8: i32) -> i32 {
let imm8 = (imm8 & 15) as u32;
simd_extract(a.as_i16x16(), imm8)
simd_extract::<_, u16>(a.as_u16x16(), imm8) as i32
}

/// Extracts a 32-bit integer from `a`, selected with `imm8`.
Expand Down Expand Up @@ -6115,7 +6115,7 @@ mod tests {
);
let r1 = _mm256_extract_epi8(a, 0);
let r2 = _mm256_extract_epi8(a, 35);
assert_eq!(r1, -1);
assert_eq!(r1, 0xFF);
assert_eq!(r2, 3);
}

Expand All @@ -6128,7 +6128,7 @@ mod tests {
);
let r1 = _mm256_extract_epi16(a, 0);
let r2 = _mm256_extract_epi16(a, 19);
assert_eq!(r1, -1);
assert_eq!(r1, 0xFFFF);
assert_eq!(r2, 3);
}

Expand Down
4 changes: 2 additions & 2 deletions crates/core_arch/src/x86/sse2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,7 @@ pub unsafe fn _mm_packus_epi16(a: __m128i, b: __m128i) -> __m128i {
#[rustc_args_required_const(1)]
#[stable(feature = "simd_x86", since = "1.27.0")]
pub unsafe fn _mm_extract_epi16(a: __m128i, imm8: i32) -> i32 {
simd_extract::<_, i16>(a.as_i16x8(), (imm8 & 7) as u32) as i32
simd_extract::<_, u16>(a.as_u16x8(), (imm8 & 7) as u32) as i32
}

/// Returns a new vector where the `imm8` element of `a` is replaced with `i`.
Expand Down Expand Up @@ -4132,7 +4132,7 @@ mod tests {
let a = _mm_setr_epi16(-1, 1, 2, 3, 4, 5, 6, 7);
let r1 = _mm_extract_epi16(a, 0);
let r2 = _mm_extract_epi16(a, 11);
assert_eq!(r1, -1);
assert_eq!(r1, 0xFFFF);
assert_eq!(r2, 3);
}

Expand Down
15 changes: 11 additions & 4 deletions crates/stdarch-verify/tests/x86-intel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ struct Data {

#[derive(Deserialize)]
struct Intrinsic {
rettype: String,
#[serde(rename = "return")]
return_: Return,
name: String,
#[serde(rename = "CPUID", default)]
cpuid: Vec<String>,
Expand All @@ -111,6 +112,12 @@ struct Parameter {
type_: String,
}

#[derive(Deserialize)]
struct Return {
#[serde(rename = "type")]
type_: String,
}

#[derive(Deserialize, Debug)]
struct Instruction {
name: String,
Expand Down Expand Up @@ -503,12 +510,12 @@ fn matches(rust: &Function, intel: &Intrinsic) -> Result<(), String> {

// Make sure we've got the right return type.
if let Some(t) = rust.ret {
equate(t, &intel.rettype, rust.name, false)?;
} else if intel.rettype != "" && intel.rettype != "void" {
equate(t, &intel.return_.type_, rust.name, false)?;
} else if intel.return_.type_ != "" && intel.return_.type_ != "void" {
bail!(
"{} returns `{}` with intel, void in rust",
rust.name,
intel.rettype
intel.return_.type_
)
}

Expand Down
Loading