Implement vec_perm #451

lu-zero · 2018-05-16T19:44:24Z

It is pretty much the opposite of vec_add: there is a single instruction vperm that works with all the possible vector types as long the operands and return type match.

The instruction has an endianness bias and it is implemented only for little endian for now.

gnzlbg · 2018-05-16T19:45:08Z

ci/run.sh

@@ -26,6 +26,7 @@ case ${TARGET} in
        export STDSIMD_DISABLE_ASSERT_INSTR=1
        ;;
    powerpc64-*)
+        export RUSTFLAGS="${RUSTFLAGS} -C target-feature=+altivec"


This should not be required

That is some stray from testing, let me remove it now.

lu-zero · 2018-05-17T11:44:41Z

Now using the arch-specific newtypes.

lu-zero · 2018-05-17T13:57:21Z

Now we also have xxpermdi so we can check how vsx instructions fare.

gnzlbg

So it's looking really good. I've left some comments, mostly nitpicks.

I am worried that #[inline] won't be enough to propagate the constant through the trait method, but I don't recall if we can use #[inline(always)] here or not.

gnzlbg · 2018-05-16T19:47:32Z

coresimd/powerpc/altivec.rs

+        }
+    }
+
+    vector_perm!{ i8x16 }


These should be implemented for the powerpc vector types, not the portable vector types. Right now they are type aliases, but the PowerPC vector types should be newtypes like on arm and mips.

gnzlbg · 2018-05-16T19:48:19Z

coresimd/powerpc/altivec.rs

+/// Vector permute.
+#[inline]
+#[target_feature(enable = "altivec")]
+pub unsafe fn vec_perm<T>(a: T, b: T, c: vector_unsigned_char) -> T


assert_instr is missing, and won't work on the generic function. You are going to have to add some functions somewhere for it to work (at that point the solution will probably look very similar to that of vec_add).

vec_vperm is the function used for the test.

gnzlbg · 2018-05-17T13:46:29Z

coresimd/powerpc/altivec.rs

+    #[inline]
+    #[target_feature(enable = "altivec")]
+    #[cfg_attr(test, assert_instr(vperm))]
+    unsafe fn vec_vperm(a: vector_signed_int, b: vector_signed_int, c: vector_unsigned_char) -> vector_signed_int {


I think we should add assert_instr tests for all vector types.

That function is the only one that is called for every vector type.

All the vector types are transmuted to i32x4.

gnzlbg · 2018-05-17T13:48:08Z

coresimd/powerpc/altivec.rs

+    T: sealed::VectorPerm,
+{
+
+    if cfg!(target_endian = "little") {


prefer conditional compilation here: #[cfg(target_endian = "little")] and #[cfg(not(target_endian = "little"))] (or #[cfg(target_endian = "big")]).

Something along the lines of

#[cfg(target_endian = "little")] mod endian { } #[cfg(target_endian = "big")] mod endian { } use self::endian::*;

gnzlbg · 2018-05-17T13:51:16Z

coresimd/powerpc/altivec.rs

@@ -666,6 +734,18 @@ mod tests {
    use simd::*;
    use stdsimd_test::simd_test;

+    #[simd_test(enable = "altivec")]
+    unsafe fn vec_perm_u16x8() {


there should be one run-time test for every vector type as well

I think that for consistency with how it is done in the other architectures we should name the run-time tests: test_{intrinsic_name}_{short_vector_types}, so this becomes test_vec_perm_u16x8.

I can macro those tests, but it would basically test that transmute works for those types.

That would be great, testing these is tedious, but sometimes optimizations happen that break this, and we also scan for assert_instr in stdsimd-verify. It would be great if the names of these functions follows a convention so that the code that scans them automatically becomes easier to write. (I followed one convention for vec_add, but maybe there is a better one).

gnzlbg · 2018-05-17T13:53:15Z

coresimd/powerpc64/vsx.rs

+    #[inline]
+    #[target_feature(enable = "vsx")]
+    #[cfg_attr(test, assert_instr(xxpermdi, dm = 0x0))]
+    unsafe fn xxpermdi(a: i64x2, b: i64x2, dm: u8) -> i64x2 {


I think this is missing a #[rustc_args_required_const(2)] to require that the user passes a compile-time constant to dm.

That won't work with the trait approach :/
crap

Maybe make this function and the trait method impls #[inline(always)] ?

The function exists mainly for the instruction test.

The const-argument is enforced on the only part that the user sees. Probably #[inline(always)] is a good idea in case the compiler decides to not const-propagate w/out the specific hint.

gnzlbg · 2018-05-17T13:57:36Z

coresimd/powerpc64/vsx.rs

+    use stdsimd_test::simd_test;
+
+    #[simd_test(enable = "vsx")]
+    unsafe fn vec_xxpermdi_u62x2() {


Also missing tests for the other vector types.

As per vec_perm, it it would test how good mem::transmute is.

Here the tests for other vector types are still missing, otherwise the rest of the PR LGTM and can be merged.

lu-zero · 2018-05-17T16:33:32Z

I refactored the endian-biased intrinsic support so it is a bit more structured.

gnzlbg · 2018-05-17T17:02:19Z

So this looks good. I'll retry the tests once CI is working again.

lu-zero · 2018-05-21T06:46:58Z

Now the set is rebased over #455

alexcrichton · 2018-05-21T19:20:57Z

That's a... curious test failure!

---- coresimd::powerpc64::vsx::tests::vec_xxpermdi_u62x2 stdout ----
thread 'coresimd::powerpc64::vsx::tests::vec_xxpermdi_u62x2' panicked at 'assertion failed: `(left == right)`
  left: `u64x2(0, 2)`,
 right: `u64x2(0, 2)`', crates/coresimd/src/../../../coresimd/powerpc64/vsx.rs:293:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

lu-zero · 2018-05-21T21:18:41Z

Yes... how do we implement equality for that type on BE?

gnzlbg · 2018-05-21T22:34:44Z

It's here: https://github.com/rust-lang-nursery/stdsimd/blob/master/coresimd/ppsv/api/partial_eq.rs#L9

We do a vertical equality comparison and then test if all bits are set.

lu-zero · 2018-05-22T06:58:00Z

I managed to find other unrelated problems while trying to reproduce the issue locally: rust-lang/rust#50960

Shall we demote powerpc and powerpc64 for the time being?

gnzlbg · 2018-05-22T07:13:15Z

Shall we demote powerpc and powerpc64 for the time being?

That was the original plan any ways. We are currently only running the assert_instr tests on ppc64le so it makes sense to only run the run-time tests there as well.

Since ppc64le is the only practical PowerPC target, I think we should just focus on that one first.

lu-zero · 2018-05-22T11:13:33Z

Now it is all green (by cheating) :)

gnzlbg · 2018-05-22T13:29:38Z

So I would prefer to just disable the run-time tests in CI e.g. by passing an environment variable that just ignores the #[simd_test]s on ppc and ppc64 instead of completely allowing them to fail. Note that all of the portable packed vector tests are currently passing on both ppc and ppc64 and I'd like to know when they break.

lu-zero · 2018-05-22T14:09:43Z

Sure, could you please point me how to do that and suggest a name for the variable?

gnzlbg · 2018-05-22T14:14:05Z

We already have STDSIMD_TEST_EVERYTHING=1 so maybe STDSIMD_TEST_NORUN=1 ?

The place is the stdsimd-test-macro (https://github.com/rust-lang-nursery/stdsimd/blob/master/crates/simd-test-macro/src/lib.rs#L108) where you want to add an #[ignore] if a particular environment variable is defined, similar to how it is done in the assert-instr-macro here: https://github.com/rust-lang-nursery/stdsimd/blob/master/crates/assert-instr-macro/src/lib.rs#L42 and here https://github.com/rust-lang-nursery/stdsimd/blob/master/crates/assert-instr-macro/src/lib.rs#L135

Bit and Little endian supported.

Otherwise some objdump output would not parse correctly.

It is what objdump produces usually.

Use STDSIMD_TEST_NORUN=1 to disable them.

lu-zero · 2018-05-22T18:06:20Z

Do you see something wrong in my travis update?

The big endian variant will be supported properly later.

lu-zero · 2018-05-22T22:08:19Z

The run script doesn't forward the environment set on travis. I put the env variable directly in the script.

gnzlbg · 2018-05-23T16:16:32Z

Thanks a lot for this! This is really good work!

gnzlbg reviewed May 16, 2018

View reviewed changes

lu-zero force-pushed the altivec branch 4 times, most recently from 47b90c2 to 20ec176 Compare May 17, 2018 11:43

gnzlbg reviewed May 17, 2018

View reviewed changes

lu-zero force-pushed the altivec branch from 2831475 to c9417d5 Compare May 17, 2018 16:30

lu-zero force-pushed the altivec branch 2 times, most recently from 6eba577 to 99bc589 Compare May 21, 2018 06:45

lu-zero force-pushed the altivec branch from 99bc589 to 8f05090 Compare May 21, 2018 18:46

lu-zero added 6 commits May 22, 2018 15:11

Implement vec_perm

31decb8

Bit and Little endian supported.

Add vec_xxpermdi

bdc6537

Add a assert_instr for vperm

6d25f83

Bump rustc_demangle version

5b981b2

Otherwise some objdump output would not parse correctly.

Check for the extended mnemonic

15beb5b

It is what objdump produces usually.

Move the endian-biased intrinsics in a separate module

fb54b14

lu-zero added 3 commits May 22, 2018 15:11

Extend the tests

c9731aa

Add multiply add saturated

59f31b5

Add an env variable to ignore the arch-specific tests

07d0cee

Use STDSIMD_TEST_NORUN=1 to disable them.

lu-zero force-pushed the altivec branch from b6f4864 to cc2bf99 Compare May 22, 2018 15:36

Do not run the altivec tests for powerpc64

b5cf336

The big endian variant will be supported properly later.

lu-zero force-pushed the altivec branch from cc2bf99 to b5cf336 Compare May 22, 2018 21:00

Extend the xxpermdi coverage

ae2ef1c

gnzlbg merged commit 137d07d into rust-lang:master May 23, 2018

Implement vec_perm #451

Implement vec_perm #451

Conversation

lu-zero commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu-zero commented May 17, 2018

lu-zero commented May 17, 2018

gnzlbg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lu-zero commented May 17, 2018

gnzlbg commented May 17, 2018

lu-zero commented May 21, 2018

alexcrichton commented May 21, 2018

lu-zero commented May 21, 2018

gnzlbg commented May 21, 2018

lu-zero commented May 22, 2018

gnzlbg commented May 22, 2018

lu-zero commented May 22, 2018

gnzlbg commented May 22, 2018

lu-zero commented May 22, 2018

gnzlbg commented May 22, 2018 • edited Loading

lu-zero commented May 22, 2018

lu-zero commented May 22, 2018

gnzlbg commented May 23, 2018

gnzlbg commented May 22, 2018 •

edited

Loading