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

x86 Various issues with extract_epiNN functions #867

Closed
jethrogb opened this issue Jun 8, 2020 · 3 comments · Fixed by #868 or #898
Closed

x86 Various issues with extract_epiNN functions #867

jethrogb opened this issue Jun 8, 2020 · 3 comments · Fixed by #868 or #898

Comments

@jethrogb
Copy link
Contributor

jethrogb commented Jun 8, 2020

1. Inconsistent function signatures

The SSE versions have the following signatures:

pub unsafe fn _mm_extract_epi8(a: __m128i, imm8: i32) -> i32;
pub unsafe fn _mm_extract_epi16(a: __m128i, imm8: i32) -> i32;

But the AVX versions have the following:

pub unsafe fn _mm256_extract_epi8(a: __m256i, imm8: i32) -> i8;
pub unsafe fn _mm256_extract_epi16(a: __m256i, imm8: i32) -> i16;

Note the difference in return type. Since Intel specifies these all return int, it's not clear how this happened. I thought the function signatures are supposed to be machine-checked?

2. Incorrect sign extension

use core::arch::x86_64::*;

fn main() {
    println!("{:x}", unsafe{_mm_extract_epi16(_mm_set1_epi8(!0), 0)});
}

This prints: ffffffff. Intel specifies the upper 16 bits should be 0.

cc @AdrianCX

@Amanieu
Copy link
Member

Amanieu commented Jun 8, 2020

The second one is definitely a bug which should be fixed.

For the first one, apparently our intrinsics data has the return type as int8/int16:

<intrinsic tech='AVX2' sequence='true' rettype='__int8' name='_mm256_extract_epi8'>
	<type>Integer</type>
	<CPUID>AVX2</CPUID>
	<category>Swizzle</category>
	<parameter varname='a' type='__m256i'/>
	<parameter varname="index" type='const int'/>
	<description>Extract an 8-bit integer from "a", selected with "index", and store the result in "dst".</description>
	<operation>
dst[7:0] := (a[255:0] &gt;&gt; (index * 8))[7:0]
	</operation>
	<header>immintrin.h</header>
</intrinsic>

@jethrogb
Copy link
Contributor Author

jethrogb commented Jun 9, 2020

Looks like https://github.com/rust-lang/stdarch/blob/master/crates/stdarch-verify/x86-intel.xml is out of date. That one's from 09/07/2017 but the most recent one is from 06/05/2020.

jethrogb pushed a commit to jethrogb/stdsimd that referenced this issue Jun 9, 2020
* Update Intel intrisics definitions with the latest version
* Update _mm256_extract_epi{8,16} to match latest definition
* Fix _mm_extract_epi16 sign extension

Fixes rust-lang#867
jethrogb pushed a commit to jethrogb/stdsimd that referenced this issue Jun 9, 2020
* Update Intel intrinsics definitions with the latest version
* Update _mm256_extract_epi{8,16} to match latest definition
* Fix _mm_extract_epi16 sign extension

Fixes rust-lang#867
Amanieu pushed a commit that referenced this issue Jun 9, 2020
* Update Intel intrinsics definitions with the latest version
* Update _mm256_extract_epi{8,16} to match latest definition
* Fix _mm_extract_epi16 sign extension

Fixes #867
@jethrogb
Copy link
Contributor Author

jethrogb commented Aug 3, 2020

Please re-open the issue (due to #878)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants