Portable vector shuffles. - #387

Closed
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles
Closed

Portable vector shuffles.#387
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles

Conversation

@gnzlbg

@gnzlbggnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

This PR implements an API for portable vector shuffles.

I've opened #388 to discuss this API.

@gnzlbggnzlbg mentioned this pull request Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Nice!

I'll admit though that I'm pretty wary about landing this, so much so that I think we'll want to keep this out of stdsimd for now if we can. I feel like the API for shuffles here is pretty up in the air (especially wrt language support), and I'm also not certain of the impact of this change once we include it in the standard library itself.

The stability of exported macros in libstd is historically a tricky topic (and even the exported traits here) and since this module will be directly included into libstd I'm hesitant to include this. I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

How critical are shuffles though to the first pass of a portable API?

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

I thought about this. Do you have a pointer to some macro that is implemented like this in rustc? I might give this a shot. About the type checking, the only thing that isn't type checked here is that the indices access the vectors in bounds (it is checked in trans). As mentioned in the comments, this should be possible in MIR/typeck, but not in librust_typeck/check/intrinsics.rs.

Also, I forgot to mention that the intrinsics should probably be annotated with the macro that checks that [T; N] is a compile-time constant in typeck instead of doing that in trans as well.

How critical are shuffles through to the first pass of a portable API?

They aren't in the first pass so they aren't critical at all. I just wanted to open an issue about a possible design, and thought that should better come with an implementation. I could add a #[cfg(feature = "stdbuild")] to the files and tests here so that these are not included in libstd builds but... i am just going to close this for now.

@gnzlbggnzlbg closed this Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Hm so thinking more implementation wise this would probably actually not be much of a procedural macro but rather almost entirely a typeck thing. In typeck we can do things like const eval and otherwise type checking so I the only reason we'd want to use a procedural macro would be to perhaps use a special AST node that can't be syntactically constructed (like asm!).

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

Before implementing that though this is probably something we'd want agreement on via an RFC before having the implementation

@gnzlbg

gnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
ContributorAuthor

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

cc @eddyb because were were talking about this a couple of hours ago. Basically if we are going to go through all that trouble, we must type check that the indices in the array of constants are in bounds. In particular, that for the one vector case they are in range [0, T::lanes()) and for the two-vectors case in range [0, 2*T::lanes()).

An alternative would be to have the macro in this PR have zero monomorphization time errors. The naive way to do that would be to not error on an index out-of-bounds in trans, and instead, insert a panic. But honestly I prefer the monomorphization-time error to that solution.

@danielrh

Copy link
Copy Markdown

Another way of approaching this is to have the function take in a trait with associated consts. I haven't found a less clunky way of doing it yet, but it could be something like this, where indices are checked at compile time:

structSIMD{pubdata:[i16;8],}macro_rules! check_indices {() => {fn check_indices(){let _test0:[u8;7 - Self::INDEX[0]] = [0;7 - Self::INDEX[0]];let _test1:[u8;7 - Self::INDEX[1]] = [0;7 - Self::INDEX[1]];let _test2:[u8;7 - Self::INDEX[2]] = [0;7 - Self::INDEX[2]];let _test3:[u8;7 - Self::INDEX[3]] = [0;7 - Self::INDEX[3]];let _test4:[u8;7 - Self::INDEX[4]] = [0;7 - Self::INDEX[4]];let _test5:[u8;7 - Self::INDEX[5]] = [0;7 - Self::INDEX[5]];let _test6:[u8;7 - Self::INDEX[6]] = [0;7 - Self::INDEX[6]];let _test7:[u8;7 - Self::INDEX[7]] = [0;7 - Self::INDEX[7]];}}}traitConstIndices{constINDEX:[usize;8];fncheck_indices();}structBackwards{}implConstIndicesforBackwards{constINDEX:[usize;8] = [7,6,5,4,3,2,1,0];check_indices!();}fnshuffle<Indices:ConstIndices>(vv:SIMD,_ind:Indices) -> SIMD{let v = vv.data;SIMD{data:[v[Indices::INDEX[0]],
v[Indices::INDEX[1]],
v[Indices::INDEX[2]],
v[Indices::INDEX[3]],
v[Indices::INDEX[4]],
v[Indices::INDEX[5]],
v[Indices::INDEX[6]],
v[Indices::INDEX[7]]]}}fnmain(){let _result = shuffle(SIMD{data:[2;8]},Backwards{});}

@gnzlbg

gnzlbg commented Apr 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Another way of approaching this is to have the function take in a trait with associated consts.

Note that the number of indices is variable: you can use shuffles to create smaller or larger vectors than the input ones:

// Given:let a:i32x8;let b:i32x8;// All of these work:let c:i32x2 = shuffle!(a, b,[3,15]);let d:i32x4 = shuffle!(a, b,[1,15,3,12]);let e:i32x16 = shuffle!(a, b,[0,1, ...,15]);

IIUC the associated const approach is going to need a little bit more work, since without const generics, the length of the associated const array cannot be generic either.

@danielrh

Copy link
Copy Markdown

Not sure this is still a good idea, but just to throw this out there with existing mechanisms: what if you had a separate function for narrowing or widening a vector that didn't take arguments (either 0 padding it or repeating it, whatever was easiest/fastest)
eg

// Given:let a:i32x8;let b:i32x8;let c:i32x2 = i32x2::prefix_trunc(shuffle!(a, b,[3,15,0,0]));let d:i32x4 = i32x4::prefix_trunc(shuffle!(a, b,[1,15,3,12,0,0,0,0]));let e:i32x16 = shuffle!(i32x16::concat(a, b),[0,1, ...,15]);

and then teach the optimizer to fuse the two shuffles you do internally
that way the type system stays simple... but it is a little more verbose than it could be.

@gnzlbg

gnzlbg commented Apr 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Ideally shuffle would be just a method on vectors with the following signature:

fnshuffle<constN:usize,R>(self,other:Self,constindices:[usize;N]) -> RwhereR:SimdVector<Item=Self::Item,Length=N>{ ...}

There are multiple problems that we currently have to face:

    1. lack of const generics
    1. lack of const function arguments

We can workaround lack of const generics by using a trait on arrays, so we can specify:

fnshuffle<I:Indices,R>(self,other:Self,constindices:I) -> RwhereR:SimdVector<Item=Self::Item,Length=I::Length>{ ...}

We can work around lack of const function arguments by making it a shuffle! macro instead, which means that we loose method position, but otherwise that's not too bad.

We could make it a very special free function (instead of a macro), by implementing it in MIR typeck as @alexcrichton suggested. There we can require that indices is an array of const items, inspect the array values to error if the indices are out-of-bounds at compile-time, etc.

So we would get a magic "function" with this signature instead:

fnshuffle<T:SimdVector,R,/*N is magic*/>(a:T,b:T,/*magically const*/indices:[usize;N]) -> RwhereR:SimdVector<Item=T::Item,Length=N>{ ...}

This PR implements it as a macro in the language, because that's basically the only way we currently have to do this with the available compiler magic, but I agree with @alexcrichton that doing this in MIR typeck is the best path forward. Maybe as the language gets const generics and const function arguments, the shuffle "function" signature can become less and less magical.

FWIW, once you have shuffle, you can implement a.concat(b) on top of it without any magic:

traitConcat:SimdVector{typeResult:SimdVector<Item=Self::Item>;fnconcat(self,other:Self) -> Self::Result;}implConcatforu32x4{typeResult = u32x8;fnconcat(self,other:u32x4) -> u32x8{shuffle!(self, other,[0,1,2,3,4,5,6,7])}}let a:u32x4;let b:u32x4;let c:u32x8 = a.concat(b);

I think that adding concat to std::simd is something worth doing, but I prefer to nail down shuffle first.

@gnzlbg

gnzlbg commented Jun 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton shall I reopen and merge this. In a nutshell, I agree that it would be better to move this macro to rustc, but I don't have the time to do it, and its API is something worth getting experience with in the meantime.

@danielrh

danielrh commented Jun 4, 2018 via email

Copy link
Copy Markdown

@gnzlbggnzlbg reopened this Jun 12, 2018
@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton maybe we could ask feedback for the lib teams on this?

Comment threadcoresimd/ppsv/api/shuffles.rs Outdated
}
};
($vec:expr, [$($l:expr),*]) => {
shuffle!($vec, $vec, [$($l),*])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will evaluate $vec twice.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Amanieu

What's the best way to fix this? Just:

{let v = $vec;shuffle!(v, v, ...)}

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

match $vec { v => shuffle!(v, v, ...) } is preferred because of let ... in ... semantics, which regular let doesn't have (less relevant here, but all the temporaries in $vec stay alive for the duration of the match).

@Amanieu

Copy link
Copy Markdown
Member

Are there any plans to support single-element vectors (e.g. u64x1)? NEON has such types and LLVM does not use the same codegen as scalar types for these (for integer types, values are kept in SIMD registers rather then being first moved to a general-purpose register).

@Amanieu

Copy link
Copy Markdown
Member

This is somewhat relevant to this issue since we will need to add a simd_shuffle1 intrinsic to support this, and I was wondering if it was worth extending this to the generic API as well.

@gnzlbg

gnzlbg commented Jun 15, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I am holding this PR until we have an idea about how to resolve: rust-lang/rfcs#2366 (comment)

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

Superseeded by https://github.com/gnzlbg/ppv

@gnzlbggnzlbg closed this Jul 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gnzlbg@alexcrichton@danielrh@Amanieu@eddyb
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Portable vector shuffles. - #387

Closed
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles
Closed

Portable vector shuffles.#387
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles

Conversation

@gnzlbg

@gnzlbggnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

This PR implements an API for portable vector shuffles.

I've opened #388 to discuss this API.

@gnzlbggnzlbg mentioned this pull request Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Nice!

I'll admit though that I'm pretty wary about landing this, so much so that I think we'll want to keep this out of stdsimd for now if we can. I feel like the API for shuffles here is pretty up in the air (especially wrt language support), and I'm also not certain of the impact of this change once we include it in the standard library itself.

The stability of exported macros in libstd is historically a tricky topic (and even the exported traits here) and since this module will be directly included into libstd I'm hesitant to include this. I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

How critical are shuffles though to the first pass of a portable API?

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

I thought about this. Do you have a pointer to some macro that is implemented like this in rustc? I might give this a shot. About the type checking, the only thing that isn't type checked here is that the indices access the vectors in bounds (it is checked in trans). As mentioned in the comments, this should be possible in MIR/typeck, but not in librust_typeck/check/intrinsics.rs.

Also, I forgot to mention that the intrinsics should probably be annotated with the macro that checks that [T; N] is a compile-time constant in typeck instead of doing that in trans as well.

How critical are shuffles through to the first pass of a portable API?

They aren't in the first pass so they aren't critical at all. I just wanted to open an issue about a possible design, and thought that should better come with an implementation. I could add a #[cfg(feature = "stdbuild")] to the files and tests here so that these are not included in libstd builds but... i am just going to close this for now.

@gnzlbggnzlbg closed this Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Hm so thinking more implementation wise this would probably actually not be much of a procedural macro but rather almost entirely a typeck thing. In typeck we can do things like const eval and otherwise type checking so I the only reason we'd want to use a procedural macro would be to perhaps use a special AST node that can't be syntactically constructed (like asm!).

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

Before implementing that though this is probably something we'd want agreement on via an RFC before having the implementation

@gnzlbg

gnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
ContributorAuthor

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

cc @eddyb because were were talking about this a couple of hours ago. Basically if we are going to go through all that trouble, we must type check that the indices in the array of constants are in bounds. In particular, that for the one vector case they are in range [0, T::lanes()) and for the two-vectors case in range [0, 2*T::lanes()).

An alternative would be to have the macro in this PR have zero monomorphization time errors. The naive way to do that would be to not error on an index out-of-bounds in trans, and instead, insert a panic. But honestly I prefer the monomorphization-time error to that solution.

@danielrh

Copy link
Copy Markdown

Another way of approaching this is to have the function take in a trait with associated consts. I haven't found a less clunky way of doing it yet, but it could be something like this, where indices are checked at compile time:

structSIMD{pubdata:[i16;8],}macro_rules! check_indices {() => {fn check_indices(){let _test0:[u8;7 - Self::INDEX[0]] = [0;7 - Self::INDEX[0]];let _test1:[u8;7 - Self::INDEX[1]] = [0;7 - Self::INDEX[1]];let _test2:[u8;7 - Self::INDEX[2]] = [0;7 - Self::INDEX[2]];let _test3:[u8;7 - Self::INDEX[3]] = [0;7 - Self::INDEX[3]];let _test4:[u8;7 - Self::INDEX[4]] = [0;7 - Self::INDEX[4]];let _test5:[u8;7 - Self::INDEX[5]] = [0;7 - Self::INDEX[5]];let _test6:[u8;7 - Self::INDEX[6]] = [0;7 - Self::INDEX[6]];let _test7:[u8;7 - Self::INDEX[7]] = [0;7 - Self::INDEX[7]];}}}traitConstIndices{constINDEX:[usize;8];fncheck_indices();}structBackwards{}implConstIndicesforBackwards{constINDEX:[usize;8] = [7,6,5,4,3,2,1,0];check_indices!();}fnshuffle<Indices:ConstIndices>(vv:SIMD,_ind:Indices) -> SIMD{let v = vv.data;SIMD{data:[v[Indices::INDEX[0]],
v[Indices::INDEX[1]],
v[Indices::INDEX[2]],
v[Indices::INDEX[3]],
v[Indices::INDEX[4]],
v[Indices::INDEX[5]],
v[Indices::INDEX[6]],
v[Indices::INDEX[7]]]}}fnmain(){let _result = shuffle(SIMD{data:[2;8]},Backwards{});}

@gnzlbg

gnzlbg commented Apr 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Another way of approaching this is to have the function take in a trait with associated consts.

Note that the number of indices is variable: you can use shuffles to create smaller or larger vectors than the input ones:

// Given:let a:i32x8;let b:i32x8;// All of these work:let c:i32x2 = shuffle!(a, b,[3,15]);let d:i32x4 = shuffle!(a, b,[1,15,3,12]);let e:i32x16 = shuffle!(a, b,[0,1, ...,15]);

IIUC the associated const approach is going to need a little bit more work, since without const generics, the length of the associated const array cannot be generic either.

@danielrh

Copy link
Copy Markdown

Not sure this is still a good idea, but just to throw this out there with existing mechanisms: what if you had a separate function for narrowing or widening a vector that didn't take arguments (either 0 padding it or repeating it, whatever was easiest/fastest)
eg

// Given:let a:i32x8;let b:i32x8;let c:i32x2 = i32x2::prefix_trunc(shuffle!(a, b,[3,15,0,0]));let d:i32x4 = i32x4::prefix_trunc(shuffle!(a, b,[1,15,3,12,0,0,0,0]));let e:i32x16 = shuffle!(i32x16::concat(a, b),[0,1, ...,15]);

and then teach the optimizer to fuse the two shuffles you do internally
that way the type system stays simple... but it is a little more verbose than it could be.

@gnzlbg

gnzlbg commented Apr 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Ideally shuffle would be just a method on vectors with the following signature:

fnshuffle<constN:usize,R>(self,other:Self,constindices:[usize;N]) -> RwhereR:SimdVector<Item=Self::Item,Length=N>{ ...}

There are multiple problems that we currently have to face:

    1. lack of const generics
    1. lack of const function arguments

We can workaround lack of const generics by using a trait on arrays, so we can specify:

fnshuffle<I:Indices,R>(self,other:Self,constindices:I) -> RwhereR:SimdVector<Item=Self::Item,Length=I::Length>{ ...}

We can work around lack of const function arguments by making it a shuffle! macro instead, which means that we loose method position, but otherwise that's not too bad.

We could make it a very special free function (instead of a macro), by implementing it in MIR typeck as @alexcrichton suggested. There we can require that indices is an array of const items, inspect the array values to error if the indices are out-of-bounds at compile-time, etc.

So we would get a magic "function" with this signature instead:

fnshuffle<T:SimdVector,R,/*N is magic*/>(a:T,b:T,/*magically const*/indices:[usize;N]) -> RwhereR:SimdVector<Item=T::Item,Length=N>{ ...}

This PR implements it as a macro in the language, because that's basically the only way we currently have to do this with the available compiler magic, but I agree with @alexcrichton that doing this in MIR typeck is the best path forward. Maybe as the language gets const generics and const function arguments, the shuffle "function" signature can become less and less magical.

FWIW, once you have shuffle, you can implement a.concat(b) on top of it without any magic:

traitConcat:SimdVector{typeResult:SimdVector<Item=Self::Item>;fnconcat(self,other:Self) -> Self::Result;}implConcatforu32x4{typeResult = u32x8;fnconcat(self,other:u32x4) -> u32x8{shuffle!(self, other,[0,1,2,3,4,5,6,7])}}let a:u32x4;let b:u32x4;let c:u32x8 = a.concat(b);

I think that adding concat to std::simd is something worth doing, but I prefer to nail down shuffle first.

@gnzlbg

gnzlbg commented Jun 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton shall I reopen and merge this. In a nutshell, I agree that it would be better to move this macro to rustc, but I don't have the time to do it, and its API is something worth getting experience with in the meantime.

@danielrh

danielrh commented Jun 4, 2018 via email

Copy link
Copy Markdown

@gnzlbggnzlbg reopened this Jun 12, 2018
@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton maybe we could ask feedback for the lib teams on this?

Comment threadcoresimd/ppsv/api/shuffles.rs Outdated
}
};
($vec:expr, [$($l:expr),*]) => {
shuffle!($vec, $vec, [$($l),*])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will evaluate $vec twice.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Amanieu

What's the best way to fix this? Just:

{let v = $vec;shuffle!(v, v, ...)}

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

match $vec { v => shuffle!(v, v, ...) } is preferred because of let ... in ... semantics, which regular let doesn't have (less relevant here, but all the temporaries in $vec stay alive for the duration of the match).

@Amanieu

Copy link
Copy Markdown
Member

Are there any plans to support single-element vectors (e.g. u64x1)? NEON has such types and LLVM does not use the same codegen as scalar types for these (for integer types, values are kept in SIMD registers rather then being first moved to a general-purpose register).

@Amanieu

Copy link
Copy Markdown
Member

This is somewhat relevant to this issue since we will need to add a simd_shuffle1 intrinsic to support this, and I was wondering if it was worth extending this to the generic API as well.

@gnzlbg

gnzlbg commented Jun 15, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I am holding this PR until we have an idea about how to resolve: rust-lang/rfcs#2366 (comment)

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

Superseeded by https://github.com/gnzlbg/ppv

@gnzlbggnzlbg closed this Jul 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gnzlbg@alexcrichton@danielrh@Amanieu@eddyb
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Portable vector shuffles. - #387

Closed
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles
Closed

Portable vector shuffles.#387
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles

Conversation

@gnzlbg

@gnzlbggnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

This PR implements an API for portable vector shuffles.

I've opened #388 to discuss this API.

@gnzlbggnzlbg mentioned this pull request Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Nice!

I'll admit though that I'm pretty wary about landing this, so much so that I think we'll want to keep this out of stdsimd for now if we can. I feel like the API for shuffles here is pretty up in the air (especially wrt language support), and I'm also not certain of the impact of this change once we include it in the standard library itself.

The stability of exported macros in libstd is historically a tricky topic (and even the exported traits here) and since this module will be directly included into libstd I'm hesitant to include this. I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

How critical are shuffles though to the first pass of a portable API?

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

I thought about this. Do you have a pointer to some macro that is implemented like this in rustc? I might give this a shot. About the type checking, the only thing that isn't type checked here is that the indices access the vectors in bounds (it is checked in trans). As mentioned in the comments, this should be possible in MIR/typeck, but not in librust_typeck/check/intrinsics.rs.

Also, I forgot to mention that the intrinsics should probably be annotated with the macro that checks that [T; N] is a compile-time constant in typeck instead of doing that in trans as well.

How critical are shuffles through to the first pass of a portable API?

They aren't in the first pass so they aren't critical at all. I just wanted to open an issue about a possible design, and thought that should better come with an implementation. I could add a #[cfg(feature = "stdbuild")] to the files and tests here so that these are not included in libstd builds but... i am just going to close this for now.

@gnzlbggnzlbg closed this Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Hm so thinking more implementation wise this would probably actually not be much of a procedural macro but rather almost entirely a typeck thing. In typeck we can do things like const eval and otherwise type checking so I the only reason we'd want to use a procedural macro would be to perhaps use a special AST node that can't be syntactically constructed (like asm!).

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

Before implementing that though this is probably something we'd want agreement on via an RFC before having the implementation

@gnzlbg

gnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
ContributorAuthor

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

cc @eddyb because were were talking about this a couple of hours ago. Basically if we are going to go through all that trouble, we must type check that the indices in the array of constants are in bounds. In particular, that for the one vector case they are in range [0, T::lanes()) and for the two-vectors case in range [0, 2*T::lanes()).

An alternative would be to have the macro in this PR have zero monomorphization time errors. The naive way to do that would be to not error on an index out-of-bounds in trans, and instead, insert a panic. But honestly I prefer the monomorphization-time error to that solution.

@danielrh

Copy link
Copy Markdown

Another way of approaching this is to have the function take in a trait with associated consts. I haven't found a less clunky way of doing it yet, but it could be something like this, where indices are checked at compile time:

structSIMD{pubdata:[i16;8],}macro_rules! check_indices {() => {fn check_indices(){let _test0:[u8;7 - Self::INDEX[0]] = [0;7 - Self::INDEX[0]];let _test1:[u8;7 - Self::INDEX[1]] = [0;7 - Self::INDEX[1]];let _test2:[u8;7 - Self::INDEX[2]] = [0;7 - Self::INDEX[2]];let _test3:[u8;7 - Self::INDEX[3]] = [0;7 - Self::INDEX[3]];let _test4:[u8;7 - Self::INDEX[4]] = [0;7 - Self::INDEX[4]];let _test5:[u8;7 - Self::INDEX[5]] = [0;7 - Self::INDEX[5]];let _test6:[u8;7 - Self::INDEX[6]] = [0;7 - Self::INDEX[6]];let _test7:[u8;7 - Self::INDEX[7]] = [0;7 - Self::INDEX[7]];}}}traitConstIndices{constINDEX:[usize;8];fncheck_indices();}structBackwards{}implConstIndicesforBackwards{constINDEX:[usize;8] = [7,6,5,4,3,2,1,0];check_indices!();}fnshuffle<Indices:ConstIndices>(vv:SIMD,_ind:Indices) -> SIMD{let v = vv.data;SIMD{data:[v[Indices::INDEX[0]],
v[Indices::INDEX[1]],
v[Indices::INDEX[2]],
v[Indices::INDEX[3]],
v[Indices::INDEX[4]],
v[Indices::INDEX[5]],
v[Indices::INDEX[6]],
v[Indices::INDEX[7]]]}}fnmain(){let _result = shuffle(SIMD{data:[2;8]},Backwards{});}

@gnzlbg

gnzlbg commented Apr 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Another way of approaching this is to have the function take in a trait with associated consts.

Note that the number of indices is variable: you can use shuffles to create smaller or larger vectors than the input ones:

// Given:let a:i32x8;let b:i32x8;// All of these work:let c:i32x2 = shuffle!(a, b,[3,15]);let d:i32x4 = shuffle!(a, b,[1,15,3,12]);let e:i32x16 = shuffle!(a, b,[0,1, ...,15]);

IIUC the associated const approach is going to need a little bit more work, since without const generics, the length of the associated const array cannot be generic either.

@danielrh

Copy link
Copy Markdown

Not sure this is still a good idea, but just to throw this out there with existing mechanisms: what if you had a separate function for narrowing or widening a vector that didn't take arguments (either 0 padding it or repeating it, whatever was easiest/fastest)
eg

// Given:let a:i32x8;let b:i32x8;let c:i32x2 = i32x2::prefix_trunc(shuffle!(a, b,[3,15,0,0]));let d:i32x4 = i32x4::prefix_trunc(shuffle!(a, b,[1,15,3,12,0,0,0,0]));let e:i32x16 = shuffle!(i32x16::concat(a, b),[0,1, ...,15]);

and then teach the optimizer to fuse the two shuffles you do internally
that way the type system stays simple... but it is a little more verbose than it could be.

@gnzlbg

gnzlbg commented Apr 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Ideally shuffle would be just a method on vectors with the following signature:

fnshuffle<constN:usize,R>(self,other:Self,constindices:[usize;N]) -> RwhereR:SimdVector<Item=Self::Item,Length=N>{ ...}

There are multiple problems that we currently have to face:

    1. lack of const generics
    1. lack of const function arguments

We can workaround lack of const generics by using a trait on arrays, so we can specify:

fnshuffle<I:Indices,R>(self,other:Self,constindices:I) -> RwhereR:SimdVector<Item=Self::Item,Length=I::Length>{ ...}

We can work around lack of const function arguments by making it a shuffle! macro instead, which means that we loose method position, but otherwise that's not too bad.

We could make it a very special free function (instead of a macro), by implementing it in MIR typeck as @alexcrichton suggested. There we can require that indices is an array of const items, inspect the array values to error if the indices are out-of-bounds at compile-time, etc.

So we would get a magic "function" with this signature instead:

fnshuffle<T:SimdVector,R,/*N is magic*/>(a:T,b:T,/*magically const*/indices:[usize;N]) -> RwhereR:SimdVector<Item=T::Item,Length=N>{ ...}

This PR implements it as a macro in the language, because that's basically the only way we currently have to do this with the available compiler magic, but I agree with @alexcrichton that doing this in MIR typeck is the best path forward. Maybe as the language gets const generics and const function arguments, the shuffle "function" signature can become less and less magical.

FWIW, once you have shuffle, you can implement a.concat(b) on top of it without any magic:

traitConcat:SimdVector{typeResult:SimdVector<Item=Self::Item>;fnconcat(self,other:Self) -> Self::Result;}implConcatforu32x4{typeResult = u32x8;fnconcat(self,other:u32x4) -> u32x8{shuffle!(self, other,[0,1,2,3,4,5,6,7])}}let a:u32x4;let b:u32x4;let c:u32x8 = a.concat(b);

I think that adding concat to std::simd is something worth doing, but I prefer to nail down shuffle first.

@gnzlbg

gnzlbg commented Jun 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton shall I reopen and merge this. In a nutshell, I agree that it would be better to move this macro to rustc, but I don't have the time to do it, and its API is something worth getting experience with in the meantime.

@danielrh

danielrh commented Jun 4, 2018 via email

Copy link
Copy Markdown

@gnzlbggnzlbg reopened this Jun 12, 2018
@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton maybe we could ask feedback for the lib teams on this?

Comment threadcoresimd/ppsv/api/shuffles.rs Outdated
}
};
($vec:expr, [$($l:expr),*]) => {
shuffle!($vec, $vec, [$($l),*])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will evaluate $vec twice.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Amanieu

What's the best way to fix this? Just:

{let v = $vec;shuffle!(v, v, ...)}

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

match $vec { v => shuffle!(v, v, ...) } is preferred because of let ... in ... semantics, which regular let doesn't have (less relevant here, but all the temporaries in $vec stay alive for the duration of the match).

@Amanieu

Copy link
Copy Markdown
Member

Are there any plans to support single-element vectors (e.g. u64x1)? NEON has such types and LLVM does not use the same codegen as scalar types for these (for integer types, values are kept in SIMD registers rather then being first moved to a general-purpose register).

@Amanieu

Copy link
Copy Markdown
Member

This is somewhat relevant to this issue since we will need to add a simd_shuffle1 intrinsic to support this, and I was wondering if it was worth extending this to the generic API as well.

@gnzlbg

gnzlbg commented Jun 15, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I am holding this PR until we have an idea about how to resolve: rust-lang/rfcs#2366 (comment)

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

Superseeded by https://github.com/gnzlbg/ppv

@gnzlbggnzlbg closed this Jul 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gnzlbg@alexcrichton@danielrh@Amanieu@eddyb
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Portable vector shuffles. - #387

Closed
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles
Closed

Portable vector shuffles.#387
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles

Conversation

@gnzlbg

@gnzlbggnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

This PR implements an API for portable vector shuffles.

I've opened #388 to discuss this API.

@gnzlbggnzlbg mentioned this pull request Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Nice!

I'll admit though that I'm pretty wary about landing this, so much so that I think we'll want to keep this out of stdsimd for now if we can. I feel like the API for shuffles here is pretty up in the air (especially wrt language support), and I'm also not certain of the impact of this change once we include it in the standard library itself.

The stability of exported macros in libstd is historically a tricky topic (and even the exported traits here) and since this module will be directly included into libstd I'm hesitant to include this. I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

How critical are shuffles though to the first pass of a portable API?

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

I thought about this. Do you have a pointer to some macro that is implemented like this in rustc? I might give this a shot. About the type checking, the only thing that isn't type checked here is that the indices access the vectors in bounds (it is checked in trans). As mentioned in the comments, this should be possible in MIR/typeck, but not in librust_typeck/check/intrinsics.rs.

Also, I forgot to mention that the intrinsics should probably be annotated with the macro that checks that [T; N] is a compile-time constant in typeck instead of doing that in trans as well.

How critical are shuffles through to the first pass of a portable API?

They aren't in the first pass so they aren't critical at all. I just wanted to open an issue about a possible design, and thought that should better come with an implementation. I could add a #[cfg(feature = "stdbuild")] to the files and tests here so that these are not included in libstd builds but... i am just going to close this for now.

@gnzlbggnzlbg closed this Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Hm so thinking more implementation wise this would probably actually not be much of a procedural macro but rather almost entirely a typeck thing. In typeck we can do things like const eval and otherwise type checking so I the only reason we'd want to use a procedural macro would be to perhaps use a special AST node that can't be syntactically constructed (like asm!).

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

Before implementing that though this is probably something we'd want agreement on via an RFC before having the implementation

@gnzlbg

gnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
ContributorAuthor

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

cc @eddyb because were were talking about this a couple of hours ago. Basically if we are going to go through all that trouble, we must type check that the indices in the array of constants are in bounds. In particular, that for the one vector case they are in range [0, T::lanes()) and for the two-vectors case in range [0, 2*T::lanes()).

An alternative would be to have the macro in this PR have zero monomorphization time errors. The naive way to do that would be to not error on an index out-of-bounds in trans, and instead, insert a panic. But honestly I prefer the monomorphization-time error to that solution.

@danielrh

Copy link
Copy Markdown

Another way of approaching this is to have the function take in a trait with associated consts. I haven't found a less clunky way of doing it yet, but it could be something like this, where indices are checked at compile time:

structSIMD{pubdata:[i16;8],}macro_rules! check_indices {() => {fn check_indices(){let _test0:[u8;7 - Self::INDEX[0]] = [0;7 - Self::INDEX[0]];let _test1:[u8;7 - Self::INDEX[1]] = [0;7 - Self::INDEX[1]];let _test2:[u8;7 - Self::INDEX[2]] = [0;7 - Self::INDEX[2]];let _test3:[u8;7 - Self::INDEX[3]] = [0;7 - Self::INDEX[3]];let _test4:[u8;7 - Self::INDEX[4]] = [0;7 - Self::INDEX[4]];let _test5:[u8;7 - Self::INDEX[5]] = [0;7 - Self::INDEX[5]];let _test6:[u8;7 - Self::INDEX[6]] = [0;7 - Self::INDEX[6]];let _test7:[u8;7 - Self::INDEX[7]] = [0;7 - Self::INDEX[7]];}}}traitConstIndices{constINDEX:[usize;8];fncheck_indices();}structBackwards{}implConstIndicesforBackwards{constINDEX:[usize;8] = [7,6,5,4,3,2,1,0];check_indices!();}fnshuffle<Indices:ConstIndices>(vv:SIMD,_ind:Indices) -> SIMD{let v = vv.data;SIMD{data:[v[Indices::INDEX[0]],
v[Indices::INDEX[1]],
v[Indices::INDEX[2]],
v[Indices::INDEX[3]],
v[Indices::INDEX[4]],
v[Indices::INDEX[5]],
v[Indices::INDEX[6]],
v[Indices::INDEX[7]]]}}fnmain(){let _result = shuffle(SIMD{data:[2;8]},Backwards{});}

@gnzlbg

gnzlbg commented Apr 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Another way of approaching this is to have the function take in a trait with associated consts.

Note that the number of indices is variable: you can use shuffles to create smaller or larger vectors than the input ones:

// Given:let a:i32x8;let b:i32x8;// All of these work:let c:i32x2 = shuffle!(a, b,[3,15]);let d:i32x4 = shuffle!(a, b,[1,15,3,12]);let e:i32x16 = shuffle!(a, b,[0,1, ...,15]);

IIUC the associated const approach is going to need a little bit more work, since without const generics, the length of the associated const array cannot be generic either.

@danielrh

Copy link
Copy Markdown

Not sure this is still a good idea, but just to throw this out there with existing mechanisms: what if you had a separate function for narrowing or widening a vector that didn't take arguments (either 0 padding it or repeating it, whatever was easiest/fastest)
eg

// Given:let a:i32x8;let b:i32x8;let c:i32x2 = i32x2::prefix_trunc(shuffle!(a, b,[3,15,0,0]));let d:i32x4 = i32x4::prefix_trunc(shuffle!(a, b,[1,15,3,12,0,0,0,0]));let e:i32x16 = shuffle!(i32x16::concat(a, b),[0,1, ...,15]);

and then teach the optimizer to fuse the two shuffles you do internally
that way the type system stays simple... but it is a little more verbose than it could be.

@gnzlbg

gnzlbg commented Apr 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Ideally shuffle would be just a method on vectors with the following signature:

fnshuffle<constN:usize,R>(self,other:Self,constindices:[usize;N]) -> RwhereR:SimdVector<Item=Self::Item,Length=N>{ ...}

There are multiple problems that we currently have to face:

    1. lack of const generics
    1. lack of const function arguments

We can workaround lack of const generics by using a trait on arrays, so we can specify:

fnshuffle<I:Indices,R>(self,other:Self,constindices:I) -> RwhereR:SimdVector<Item=Self::Item,Length=I::Length>{ ...}

We can work around lack of const function arguments by making it a shuffle! macro instead, which means that we loose method position, but otherwise that's not too bad.

We could make it a very special free function (instead of a macro), by implementing it in MIR typeck as @alexcrichton suggested. There we can require that indices is an array of const items, inspect the array values to error if the indices are out-of-bounds at compile-time, etc.

So we would get a magic "function" with this signature instead:

fnshuffle<T:SimdVector,R,/*N is magic*/>(a:T,b:T,/*magically const*/indices:[usize;N]) -> RwhereR:SimdVector<Item=T::Item,Length=N>{ ...}

This PR implements it as a macro in the language, because that's basically the only way we currently have to do this with the available compiler magic, but I agree with @alexcrichton that doing this in MIR typeck is the best path forward. Maybe as the language gets const generics and const function arguments, the shuffle "function" signature can become less and less magical.

FWIW, once you have shuffle, you can implement a.concat(b) on top of it without any magic:

traitConcat:SimdVector{typeResult:SimdVector<Item=Self::Item>;fnconcat(self,other:Self) -> Self::Result;}implConcatforu32x4{typeResult = u32x8;fnconcat(self,other:u32x4) -> u32x8{shuffle!(self, other,[0,1,2,3,4,5,6,7])}}let a:u32x4;let b:u32x4;let c:u32x8 = a.concat(b);

I think that adding concat to std::simd is something worth doing, but I prefer to nail down shuffle first.

@gnzlbg

gnzlbg commented Jun 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton shall I reopen and merge this. In a nutshell, I agree that it would be better to move this macro to rustc, but I don't have the time to do it, and its API is something worth getting experience with in the meantime.

@danielrh

danielrh commented Jun 4, 2018 via email

Copy link
Copy Markdown

@gnzlbggnzlbg reopened this Jun 12, 2018
@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton maybe we could ask feedback for the lib teams on this?

Comment threadcoresimd/ppsv/api/shuffles.rs Outdated
}
};
($vec:expr, [$($l:expr),*]) => {
shuffle!($vec, $vec, [$($l),*])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will evaluate $vec twice.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Amanieu

What's the best way to fix this? Just:

{let v = $vec;shuffle!(v, v, ...)}

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

match $vec { v => shuffle!(v, v, ...) } is preferred because of let ... in ... semantics, which regular let doesn't have (less relevant here, but all the temporaries in $vec stay alive for the duration of the match).

@Amanieu

Copy link
Copy Markdown
Member

Are there any plans to support single-element vectors (e.g. u64x1)? NEON has such types and LLVM does not use the same codegen as scalar types for these (for integer types, values are kept in SIMD registers rather then being first moved to a general-purpose register).

@Amanieu

Copy link
Copy Markdown
Member

This is somewhat relevant to this issue since we will need to add a simd_shuffle1 intrinsic to support this, and I was wondering if it was worth extending this to the generic API as well.

@gnzlbg

gnzlbg commented Jun 15, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I am holding this PR until we have an idea about how to resolve: rust-lang/rfcs#2366 (comment)

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

Superseeded by https://github.com/gnzlbg/ppv

@gnzlbggnzlbg closed this Jul 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gnzlbg@alexcrichton@danielrh@Amanieu@eddyb
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Portable vector shuffles. - #387

Closed
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles
Closed

Portable vector shuffles.#387
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles

Conversation

@gnzlbg

@gnzlbggnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

This PR implements an API for portable vector shuffles.

I've opened #388 to discuss this API.

@gnzlbggnzlbg mentioned this pull request Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Nice!

I'll admit though that I'm pretty wary about landing this, so much so that I think we'll want to keep this out of stdsimd for now if we can. I feel like the API for shuffles here is pretty up in the air (especially wrt language support), and I'm also not certain of the impact of this change once we include it in the standard library itself.

The stability of exported macros in libstd is historically a tricky topic (and even the exported traits here) and since this module will be directly included into libstd I'm hesitant to include this. I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

How critical are shuffles though to the first pass of a portable API?

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

I thought about this. Do you have a pointer to some macro that is implemented like this in rustc? I might give this a shot. About the type checking, the only thing that isn't type checked here is that the indices access the vectors in bounds (it is checked in trans). As mentioned in the comments, this should be possible in MIR/typeck, but not in librust_typeck/check/intrinsics.rs.

Also, I forgot to mention that the intrinsics should probably be annotated with the macro that checks that [T; N] is a compile-time constant in typeck instead of doing that in trans as well.

How critical are shuffles through to the first pass of a portable API?

They aren't in the first pass so they aren't critical at all. I just wanted to open an issue about a possible design, and thought that should better come with an implementation. I could add a #[cfg(feature = "stdbuild")] to the files and tests here so that these are not included in libstd builds but... i am just going to close this for now.

@gnzlbggnzlbg closed this Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Hm so thinking more implementation wise this would probably actually not be much of a procedural macro but rather almost entirely a typeck thing. In typeck we can do things like const eval and otherwise type checking so I the only reason we'd want to use a procedural macro would be to perhaps use a special AST node that can't be syntactically constructed (like asm!).

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

Before implementing that though this is probably something we'd want agreement on via an RFC before having the implementation

@gnzlbg

gnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
ContributorAuthor

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

cc @eddyb because were were talking about this a couple of hours ago. Basically if we are going to go through all that trouble, we must type check that the indices in the array of constants are in bounds. In particular, that for the one vector case they are in range [0, T::lanes()) and for the two-vectors case in range [0, 2*T::lanes()).

An alternative would be to have the macro in this PR have zero monomorphization time errors. The naive way to do that would be to not error on an index out-of-bounds in trans, and instead, insert a panic. But honestly I prefer the monomorphization-time error to that solution.

@danielrh

Copy link
Copy Markdown

Another way of approaching this is to have the function take in a trait with associated consts. I haven't found a less clunky way of doing it yet, but it could be something like this, where indices are checked at compile time:

structSIMD{pubdata:[i16;8],}macro_rules! check_indices {() => {fn check_indices(){let _test0:[u8;7 - Self::INDEX[0]] = [0;7 - Self::INDEX[0]];let _test1:[u8;7 - Self::INDEX[1]] = [0;7 - Self::INDEX[1]];let _test2:[u8;7 - Self::INDEX[2]] = [0;7 - Self::INDEX[2]];let _test3:[u8;7 - Self::INDEX[3]] = [0;7 - Self::INDEX[3]];let _test4:[u8;7 - Self::INDEX[4]] = [0;7 - Self::INDEX[4]];let _test5:[u8;7 - Self::INDEX[5]] = [0;7 - Self::INDEX[5]];let _test6:[u8;7 - Self::INDEX[6]] = [0;7 - Self::INDEX[6]];let _test7:[u8;7 - Self::INDEX[7]] = [0;7 - Self::INDEX[7]];}}}traitConstIndices{constINDEX:[usize;8];fncheck_indices();}structBackwards{}implConstIndicesforBackwards{constINDEX:[usize;8] = [7,6,5,4,3,2,1,0];check_indices!();}fnshuffle<Indices:ConstIndices>(vv:SIMD,_ind:Indices) -> SIMD{let v = vv.data;SIMD{data:[v[Indices::INDEX[0]],
v[Indices::INDEX[1]],
v[Indices::INDEX[2]],
v[Indices::INDEX[3]],
v[Indices::INDEX[4]],
v[Indices::INDEX[5]],
v[Indices::INDEX[6]],
v[Indices::INDEX[7]]]}}fnmain(){let _result = shuffle(SIMD{data:[2;8]},Backwards{});}

@gnzlbg

gnzlbg commented Apr 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Another way of approaching this is to have the function take in a trait with associated consts.

Note that the number of indices is variable: you can use shuffles to create smaller or larger vectors than the input ones:

// Given:let a:i32x8;let b:i32x8;// All of these work:let c:i32x2 = shuffle!(a, b,[3,15]);let d:i32x4 = shuffle!(a, b,[1,15,3,12]);let e:i32x16 = shuffle!(a, b,[0,1, ...,15]);

IIUC the associated const approach is going to need a little bit more work, since without const generics, the length of the associated const array cannot be generic either.

@danielrh

Copy link
Copy Markdown

Not sure this is still a good idea, but just to throw this out there with existing mechanisms: what if you had a separate function for narrowing or widening a vector that didn't take arguments (either 0 padding it or repeating it, whatever was easiest/fastest)
eg

// Given:let a:i32x8;let b:i32x8;let c:i32x2 = i32x2::prefix_trunc(shuffle!(a, b,[3,15,0,0]));let d:i32x4 = i32x4::prefix_trunc(shuffle!(a, b,[1,15,3,12,0,0,0,0]));let e:i32x16 = shuffle!(i32x16::concat(a, b),[0,1, ...,15]);

and then teach the optimizer to fuse the two shuffles you do internally
that way the type system stays simple... but it is a little more verbose than it could be.

@gnzlbg

gnzlbg commented Apr 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Ideally shuffle would be just a method on vectors with the following signature:

fnshuffle<constN:usize,R>(self,other:Self,constindices:[usize;N]) -> RwhereR:SimdVector<Item=Self::Item,Length=N>{ ...}

There are multiple problems that we currently have to face:

    1. lack of const generics
    1. lack of const function arguments

We can workaround lack of const generics by using a trait on arrays, so we can specify:

fnshuffle<I:Indices,R>(self,other:Self,constindices:I) -> RwhereR:SimdVector<Item=Self::Item,Length=I::Length>{ ...}

We can work around lack of const function arguments by making it a shuffle! macro instead, which means that we loose method position, but otherwise that's not too bad.

We could make it a very special free function (instead of a macro), by implementing it in MIR typeck as @alexcrichton suggested. There we can require that indices is an array of const items, inspect the array values to error if the indices are out-of-bounds at compile-time, etc.

So we would get a magic "function" with this signature instead:

fnshuffle<T:SimdVector,R,/*N is magic*/>(a:T,b:T,/*magically const*/indices:[usize;N]) -> RwhereR:SimdVector<Item=T::Item,Length=N>{ ...}

This PR implements it as a macro in the language, because that's basically the only way we currently have to do this with the available compiler magic, but I agree with @alexcrichton that doing this in MIR typeck is the best path forward. Maybe as the language gets const generics and const function arguments, the shuffle "function" signature can become less and less magical.

FWIW, once you have shuffle, you can implement a.concat(b) on top of it without any magic:

traitConcat:SimdVector{typeResult:SimdVector<Item=Self::Item>;fnconcat(self,other:Self) -> Self::Result;}implConcatforu32x4{typeResult = u32x8;fnconcat(self,other:u32x4) -> u32x8{shuffle!(self, other,[0,1,2,3,4,5,6,7])}}let a:u32x4;let b:u32x4;let c:u32x8 = a.concat(b);

I think that adding concat to std::simd is something worth doing, but I prefer to nail down shuffle first.

@gnzlbg

gnzlbg commented Jun 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton shall I reopen and merge this. In a nutshell, I agree that it would be better to move this macro to rustc, but I don't have the time to do it, and its API is something worth getting experience with in the meantime.

@danielrh

danielrh commented Jun 4, 2018 via email

Copy link
Copy Markdown

@gnzlbggnzlbg reopened this Jun 12, 2018
@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton maybe we could ask feedback for the lib teams on this?

Comment threadcoresimd/ppsv/api/shuffles.rs Outdated
}
};
($vec:expr, [$($l:expr),*]) => {
shuffle!($vec, $vec, [$($l),*])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will evaluate $vec twice.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Amanieu

What's the best way to fix this? Just:

{let v = $vec;shuffle!(v, v, ...)}

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

match $vec { v => shuffle!(v, v, ...) } is preferred because of let ... in ... semantics, which regular let doesn't have (less relevant here, but all the temporaries in $vec stay alive for the duration of the match).

@Amanieu

Copy link
Copy Markdown
Member

Are there any plans to support single-element vectors (e.g. u64x1)? NEON has such types and LLVM does not use the same codegen as scalar types for these (for integer types, values are kept in SIMD registers rather then being first moved to a general-purpose register).

@Amanieu

Copy link
Copy Markdown
Member

This is somewhat relevant to this issue since we will need to add a simd_shuffle1 intrinsic to support this, and I was wondering if it was worth extending this to the generic API as well.

@gnzlbg

gnzlbg commented Jun 15, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I am holding this PR until we have an idea about how to resolve: rust-lang/rfcs#2366 (comment)

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

Superseeded by https://github.com/gnzlbg/ppv

@gnzlbggnzlbg closed this Jul 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gnzlbg@alexcrichton@danielrh@Amanieu@eddyb
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Portable vector shuffles. - #387

Closed
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles
Closed

Portable vector shuffles.#387
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles

Conversation

@gnzlbg

@gnzlbggnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

This PR implements an API for portable vector shuffles.

I've opened #388 to discuss this API.

@gnzlbggnzlbg mentioned this pull request Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Nice!

I'll admit though that I'm pretty wary about landing this, so much so that I think we'll want to keep this out of stdsimd for now if we can. I feel like the API for shuffles here is pretty up in the air (especially wrt language support), and I'm also not certain of the impact of this change once we include it in the standard library itself.

The stability of exported macros in libstd is historically a tricky topic (and even the exported traits here) and since this module will be directly included into libstd I'm hesitant to include this. I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

How critical are shuffles though to the first pass of a portable API?

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

I thought about this. Do you have a pointer to some macro that is implemented like this in rustc? I might give this a shot. About the type checking, the only thing that isn't type checked here is that the indices access the vectors in bounds (it is checked in trans). As mentioned in the comments, this should be possible in MIR/typeck, but not in librust_typeck/check/intrinsics.rs.

Also, I forgot to mention that the intrinsics should probably be annotated with the macro that checks that [T; N] is a compile-time constant in typeck instead of doing that in trans as well.

How critical are shuffles through to the first pass of a portable API?

They aren't in the first pass so they aren't critical at all. I just wanted to open an issue about a possible design, and thought that should better come with an implementation. I could add a #[cfg(feature = "stdbuild")] to the files and tests here so that these are not included in libstd builds but... i am just going to close this for now.

@gnzlbggnzlbg closed this Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Hm so thinking more implementation wise this would probably actually not be much of a procedural macro but rather almost entirely a typeck thing. In typeck we can do things like const eval and otherwise type checking so I the only reason we'd want to use a procedural macro would be to perhaps use a special AST node that can't be syntactically constructed (like asm!).

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

Before implementing that though this is probably something we'd want agreement on via an RFC before having the implementation

@gnzlbg

gnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
ContributorAuthor

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

cc @eddyb because were were talking about this a couple of hours ago. Basically if we are going to go through all that trouble, we must type check that the indices in the array of constants are in bounds. In particular, that for the one vector case they are in range [0, T::lanes()) and for the two-vectors case in range [0, 2*T::lanes()).

An alternative would be to have the macro in this PR have zero monomorphization time errors. The naive way to do that would be to not error on an index out-of-bounds in trans, and instead, insert a panic. But honestly I prefer the monomorphization-time error to that solution.

@danielrh

Copy link
Copy Markdown

Another way of approaching this is to have the function take in a trait with associated consts. I haven't found a less clunky way of doing it yet, but it could be something like this, where indices are checked at compile time:

structSIMD{pubdata:[i16;8],}macro_rules! check_indices {() => {fn check_indices(){let _test0:[u8;7 - Self::INDEX[0]] = [0;7 - Self::INDEX[0]];let _test1:[u8;7 - Self::INDEX[1]] = [0;7 - Self::INDEX[1]];let _test2:[u8;7 - Self::INDEX[2]] = [0;7 - Self::INDEX[2]];let _test3:[u8;7 - Self::INDEX[3]] = [0;7 - Self::INDEX[3]];let _test4:[u8;7 - Self::INDEX[4]] = [0;7 - Self::INDEX[4]];let _test5:[u8;7 - Self::INDEX[5]] = [0;7 - Self::INDEX[5]];let _test6:[u8;7 - Self::INDEX[6]] = [0;7 - Self::INDEX[6]];let _test7:[u8;7 - Self::INDEX[7]] = [0;7 - Self::INDEX[7]];}}}traitConstIndices{constINDEX:[usize;8];fncheck_indices();}structBackwards{}implConstIndicesforBackwards{constINDEX:[usize;8] = [7,6,5,4,3,2,1,0];check_indices!();}fnshuffle<Indices:ConstIndices>(vv:SIMD,_ind:Indices) -> SIMD{let v = vv.data;SIMD{data:[v[Indices::INDEX[0]],
v[Indices::INDEX[1]],
v[Indices::INDEX[2]],
v[Indices::INDEX[3]],
v[Indices::INDEX[4]],
v[Indices::INDEX[5]],
v[Indices::INDEX[6]],
v[Indices::INDEX[7]]]}}fnmain(){let _result = shuffle(SIMD{data:[2;8]},Backwards{});}

@gnzlbg

gnzlbg commented Apr 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Another way of approaching this is to have the function take in a trait with associated consts.

Note that the number of indices is variable: you can use shuffles to create smaller or larger vectors than the input ones:

// Given:let a:i32x8;let b:i32x8;// All of these work:let c:i32x2 = shuffle!(a, b,[3,15]);let d:i32x4 = shuffle!(a, b,[1,15,3,12]);let e:i32x16 = shuffle!(a, b,[0,1, ...,15]);

IIUC the associated const approach is going to need a little bit more work, since without const generics, the length of the associated const array cannot be generic either.

@danielrh

Copy link
Copy Markdown

Not sure this is still a good idea, but just to throw this out there with existing mechanisms: what if you had a separate function for narrowing or widening a vector that didn't take arguments (either 0 padding it or repeating it, whatever was easiest/fastest)
eg

// Given:let a:i32x8;let b:i32x8;let c:i32x2 = i32x2::prefix_trunc(shuffle!(a, b,[3,15,0,0]));let d:i32x4 = i32x4::prefix_trunc(shuffle!(a, b,[1,15,3,12,0,0,0,0]));let e:i32x16 = shuffle!(i32x16::concat(a, b),[0,1, ...,15]);

and then teach the optimizer to fuse the two shuffles you do internally
that way the type system stays simple... but it is a little more verbose than it could be.

@gnzlbg

gnzlbg commented Apr 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Ideally shuffle would be just a method on vectors with the following signature:

fnshuffle<constN:usize,R>(self,other:Self,constindices:[usize;N]) -> RwhereR:SimdVector<Item=Self::Item,Length=N>{ ...}

There are multiple problems that we currently have to face:

    1. lack of const generics
    1. lack of const function arguments

We can workaround lack of const generics by using a trait on arrays, so we can specify:

fnshuffle<I:Indices,R>(self,other:Self,constindices:I) -> RwhereR:SimdVector<Item=Self::Item,Length=I::Length>{ ...}

We can work around lack of const function arguments by making it a shuffle! macro instead, which means that we loose method position, but otherwise that's not too bad.

We could make it a very special free function (instead of a macro), by implementing it in MIR typeck as @alexcrichton suggested. There we can require that indices is an array of const items, inspect the array values to error if the indices are out-of-bounds at compile-time, etc.

So we would get a magic "function" with this signature instead:

fnshuffle<T:SimdVector,R,/*N is magic*/>(a:T,b:T,/*magically const*/indices:[usize;N]) -> RwhereR:SimdVector<Item=T::Item,Length=N>{ ...}

This PR implements it as a macro in the language, because that's basically the only way we currently have to do this with the available compiler magic, but I agree with @alexcrichton that doing this in MIR typeck is the best path forward. Maybe as the language gets const generics and const function arguments, the shuffle "function" signature can become less and less magical.

FWIW, once you have shuffle, you can implement a.concat(b) on top of it without any magic:

traitConcat:SimdVector{typeResult:SimdVector<Item=Self::Item>;fnconcat(self,other:Self) -> Self::Result;}implConcatforu32x4{typeResult = u32x8;fnconcat(self,other:u32x4) -> u32x8{shuffle!(self, other,[0,1,2,3,4,5,6,7])}}let a:u32x4;let b:u32x4;let c:u32x8 = a.concat(b);

I think that adding concat to std::simd is something worth doing, but I prefer to nail down shuffle first.

@gnzlbg

gnzlbg commented Jun 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton shall I reopen and merge this. In a nutshell, I agree that it would be better to move this macro to rustc, but I don't have the time to do it, and its API is something worth getting experience with in the meantime.

@danielrh

danielrh commented Jun 4, 2018 via email

Copy link
Copy Markdown

@gnzlbggnzlbg reopened this Jun 12, 2018
@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton maybe we could ask feedback for the lib teams on this?

Comment threadcoresimd/ppsv/api/shuffles.rs Outdated
}
};
($vec:expr, [$($l:expr),*]) => {
shuffle!($vec, $vec, [$($l),*])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will evaluate $vec twice.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Amanieu

What's the best way to fix this? Just:

{let v = $vec;shuffle!(v, v, ...)}

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

match $vec { v => shuffle!(v, v, ...) } is preferred because of let ... in ... semantics, which regular let doesn't have (less relevant here, but all the temporaries in $vec stay alive for the duration of the match).

@Amanieu

Copy link
Copy Markdown
Member

Are there any plans to support single-element vectors (e.g. u64x1)? NEON has such types and LLVM does not use the same codegen as scalar types for these (for integer types, values are kept in SIMD registers rather then being first moved to a general-purpose register).

@Amanieu

Copy link
Copy Markdown
Member

This is somewhat relevant to this issue since we will need to add a simd_shuffle1 intrinsic to support this, and I was wondering if it was worth extending this to the generic API as well.

@gnzlbg

gnzlbg commented Jun 15, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I am holding this PR until we have an idea about how to resolve: rust-lang/rfcs#2366 (comment)

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

Superseeded by https://github.com/gnzlbg/ppv

@gnzlbggnzlbg closed this Jul 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gnzlbg@alexcrichton@danielrh@Amanieu@eddyb
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Portable vector shuffles. - #387

Closed
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles
Closed

Portable vector shuffles.#387
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles

Conversation

@gnzlbg

@gnzlbggnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

This PR implements an API for portable vector shuffles.

I've opened #388 to discuss this API.

@gnzlbggnzlbg mentioned this pull request Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Nice!

I'll admit though that I'm pretty wary about landing this, so much so that I think we'll want to keep this out of stdsimd for now if we can. I feel like the API for shuffles here is pretty up in the air (especially wrt language support), and I'm also not certain of the impact of this change once we include it in the standard library itself.

The stability of exported macros in libstd is historically a tricky topic (and even the exported traits here) and since this module will be directly included into libstd I'm hesitant to include this. I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

How critical are shuffles though to the first pass of a portable API?

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

I thought about this. Do you have a pointer to some macro that is implemented like this in rustc? I might give this a shot. About the type checking, the only thing that isn't type checked here is that the indices access the vectors in bounds (it is checked in trans). As mentioned in the comments, this should be possible in MIR/typeck, but not in librust_typeck/check/intrinsics.rs.

Also, I forgot to mention that the intrinsics should probably be annotated with the macro that checks that [T; N] is a compile-time constant in typeck instead of doing that in trans as well.

How critical are shuffles through to the first pass of a portable API?

They aren't in the first pass so they aren't critical at all. I just wanted to open an issue about a possible design, and thought that should better come with an implementation. I could add a #[cfg(feature = "stdbuild")] to the files and tests here so that these are not included in libstd builds but... i am just going to close this for now.

@gnzlbggnzlbg closed this Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Hm so thinking more implementation wise this would probably actually not be much of a procedural macro but rather almost entirely a typeck thing. In typeck we can do things like const eval and otherwise type checking so I the only reason we'd want to use a procedural macro would be to perhaps use a special AST node that can't be syntactically constructed (like asm!).

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

Before implementing that though this is probably something we'd want agreement on via an RFC before having the implementation

@gnzlbg

gnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
ContributorAuthor

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

cc @eddyb because were were talking about this a couple of hours ago. Basically if we are going to go through all that trouble, we must type check that the indices in the array of constants are in bounds. In particular, that for the one vector case they are in range [0, T::lanes()) and for the two-vectors case in range [0, 2*T::lanes()).

An alternative would be to have the macro in this PR have zero monomorphization time errors. The naive way to do that would be to not error on an index out-of-bounds in trans, and instead, insert a panic. But honestly I prefer the monomorphization-time error to that solution.

@danielrh

Copy link
Copy Markdown

Another way of approaching this is to have the function take in a trait with associated consts. I haven't found a less clunky way of doing it yet, but it could be something like this, where indices are checked at compile time:

structSIMD{pubdata:[i16;8],}macro_rules! check_indices {() => {fn check_indices(){let _test0:[u8;7 - Self::INDEX[0]] = [0;7 - Self::INDEX[0]];let _test1:[u8;7 - Self::INDEX[1]] = [0;7 - Self::INDEX[1]];let _test2:[u8;7 - Self::INDEX[2]] = [0;7 - Self::INDEX[2]];let _test3:[u8;7 - Self::INDEX[3]] = [0;7 - Self::INDEX[3]];let _test4:[u8;7 - Self::INDEX[4]] = [0;7 - Self::INDEX[4]];let _test5:[u8;7 - Self::INDEX[5]] = [0;7 - Self::INDEX[5]];let _test6:[u8;7 - Self::INDEX[6]] = [0;7 - Self::INDEX[6]];let _test7:[u8;7 - Self::INDEX[7]] = [0;7 - Self::INDEX[7]];}}}traitConstIndices{constINDEX:[usize;8];fncheck_indices();}structBackwards{}implConstIndicesforBackwards{constINDEX:[usize;8] = [7,6,5,4,3,2,1,0];check_indices!();}fnshuffle<Indices:ConstIndices>(vv:SIMD,_ind:Indices) -> SIMD{let v = vv.data;SIMD{data:[v[Indices::INDEX[0]],
v[Indices::INDEX[1]],
v[Indices::INDEX[2]],
v[Indices::INDEX[3]],
v[Indices::INDEX[4]],
v[Indices::INDEX[5]],
v[Indices::INDEX[6]],
v[Indices::INDEX[7]]]}}fnmain(){let _result = shuffle(SIMD{data:[2;8]},Backwards{});}

@gnzlbg

gnzlbg commented Apr 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Another way of approaching this is to have the function take in a trait with associated consts.

Note that the number of indices is variable: you can use shuffles to create smaller or larger vectors than the input ones:

// Given:let a:i32x8;let b:i32x8;// All of these work:let c:i32x2 = shuffle!(a, b,[3,15]);let d:i32x4 = shuffle!(a, b,[1,15,3,12]);let e:i32x16 = shuffle!(a, b,[0,1, ...,15]);

IIUC the associated const approach is going to need a little bit more work, since without const generics, the length of the associated const array cannot be generic either.

@danielrh

Copy link
Copy Markdown

Not sure this is still a good idea, but just to throw this out there with existing mechanisms: what if you had a separate function for narrowing or widening a vector that didn't take arguments (either 0 padding it or repeating it, whatever was easiest/fastest)
eg

// Given:let a:i32x8;let b:i32x8;let c:i32x2 = i32x2::prefix_trunc(shuffle!(a, b,[3,15,0,0]));let d:i32x4 = i32x4::prefix_trunc(shuffle!(a, b,[1,15,3,12,0,0,0,0]));let e:i32x16 = shuffle!(i32x16::concat(a, b),[0,1, ...,15]);

and then teach the optimizer to fuse the two shuffles you do internally
that way the type system stays simple... but it is a little more verbose than it could be.

@gnzlbg

gnzlbg commented Apr 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Ideally shuffle would be just a method on vectors with the following signature:

fnshuffle<constN:usize,R>(self,other:Self,constindices:[usize;N]) -> RwhereR:SimdVector<Item=Self::Item,Length=N>{ ...}

There are multiple problems that we currently have to face:

    1. lack of const generics
    1. lack of const function arguments

We can workaround lack of const generics by using a trait on arrays, so we can specify:

fnshuffle<I:Indices,R>(self,other:Self,constindices:I) -> RwhereR:SimdVector<Item=Self::Item,Length=I::Length>{ ...}

We can work around lack of const function arguments by making it a shuffle! macro instead, which means that we loose method position, but otherwise that's not too bad.

We could make it a very special free function (instead of a macro), by implementing it in MIR typeck as @alexcrichton suggested. There we can require that indices is an array of const items, inspect the array values to error if the indices are out-of-bounds at compile-time, etc.

So we would get a magic "function" with this signature instead:

fnshuffle<T:SimdVector,R,/*N is magic*/>(a:T,b:T,/*magically const*/indices:[usize;N]) -> RwhereR:SimdVector<Item=T::Item,Length=N>{ ...}

This PR implements it as a macro in the language, because that's basically the only way we currently have to do this with the available compiler magic, but I agree with @alexcrichton that doing this in MIR typeck is the best path forward. Maybe as the language gets const generics and const function arguments, the shuffle "function" signature can become less and less magical.

FWIW, once you have shuffle, you can implement a.concat(b) on top of it without any magic:

traitConcat:SimdVector{typeResult:SimdVector<Item=Self::Item>;fnconcat(self,other:Self) -> Self::Result;}implConcatforu32x4{typeResult = u32x8;fnconcat(self,other:u32x4) -> u32x8{shuffle!(self, other,[0,1,2,3,4,5,6,7])}}let a:u32x4;let b:u32x4;let c:u32x8 = a.concat(b);

I think that adding concat to std::simd is something worth doing, but I prefer to nail down shuffle first.

@gnzlbg

gnzlbg commented Jun 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton shall I reopen and merge this. In a nutshell, I agree that it would be better to move this macro to rustc, but I don't have the time to do it, and its API is something worth getting experience with in the meantime.

@danielrh

danielrh commented Jun 4, 2018 via email

Copy link
Copy Markdown

@gnzlbggnzlbg reopened this Jun 12, 2018
@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton maybe we could ask feedback for the lib teams on this?

Comment threadcoresimd/ppsv/api/shuffles.rs Outdated
}
};
($vec:expr, [$($l:expr),*]) => {
shuffle!($vec, $vec, [$($l),*])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will evaluate $vec twice.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Amanieu

What's the best way to fix this? Just:

{let v = $vec;shuffle!(v, v, ...)}

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

match $vec { v => shuffle!(v, v, ...) } is preferred because of let ... in ... semantics, which regular let doesn't have (less relevant here, but all the temporaries in $vec stay alive for the duration of the match).

@Amanieu

Copy link
Copy Markdown
Member

Are there any plans to support single-element vectors (e.g. u64x1)? NEON has such types and LLVM does not use the same codegen as scalar types for these (for integer types, values are kept in SIMD registers rather then being first moved to a general-purpose register).

@Amanieu

Copy link
Copy Markdown
Member

This is somewhat relevant to this issue since we will need to add a simd_shuffle1 intrinsic to support this, and I was wondering if it was worth extending this to the generic API as well.

@gnzlbg

gnzlbg commented Jun 15, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I am holding this PR until we have an idea about how to resolve: rust-lang/rfcs#2366 (comment)

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

Superseeded by https://github.com/gnzlbg/ppv

@gnzlbggnzlbg closed this Jul 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gnzlbg@alexcrichton@danielrh@Amanieu@eddyb
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Portable vector shuffles. - #387

Closed
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles
Closed

Portable vector shuffles.#387
gnzlbg wants to merge 4 commits into
rust-lang:masterfrom
gnzlbg:shuffles

Conversation

@gnzlbg

@gnzlbggnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

This PR implements an API for portable vector shuffles.

I've opened #388 to discuss this API.

@gnzlbggnzlbg mentioned this pull request Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Nice!

I'll admit though that I'm pretty wary about landing this, so much so that I think we'll want to keep this out of stdsimd for now if we can. I feel like the API for shuffles here is pretty up in the air (especially wrt language support), and I'm also not certain of the impact of this change once we include it in the standard library itself.

The stability of exported macros in libstd is historically a tricky topic (and even the exported traits here) and since this module will be directly included into libstd I'm hesitant to include this. I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

How critical are shuffles though to the first pass of a portable API?

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I feel like in the long run we'll either want this to be a procedural macro in rustc and also have more MIR/typeck support for preventing errors at compile time.

I thought about this. Do you have a pointer to some macro that is implemented like this in rustc? I might give this a shot. About the type checking, the only thing that isn't type checked here is that the indices access the vectors in bounds (it is checked in trans). As mentioned in the comments, this should be possible in MIR/typeck, but not in librust_typeck/check/intrinsics.rs.

Also, I forgot to mention that the intrinsics should probably be annotated with the macro that checks that [T; N] is a compile-time constant in typeck instead of doing that in trans as well.

How critical are shuffles through to the first pass of a portable API?

They aren't in the first pass so they aren't critical at all. I just wanted to open an issue about a possible design, and thought that should better come with an implementation. I could add a #[cfg(feature = "stdbuild")] to the files and tests here so that these are not included in libstd builds but... i am just going to close this for now.

@gnzlbggnzlbg closed this Mar 20, 2018
@alexcrichton

Copy link
Copy Markdown
Member

Hm so thinking more implementation wise this would probably actually not be much of a procedural macro but rather almost entirely a typeck thing. In typeck we can do things like const eval and otherwise type checking so I the only reason we'd want to use a procedural macro would be to perhaps use a special AST node that can't be syntactically constructed (like asm!).

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

Before implementing that though this is probably something we'd want agreement on via an RFC before having the implementation

@gnzlbg

gnzlbg commented Mar 20, 2018

Copy link
Copy Markdown
ContributorAuthor

In that sense it may make sense to just leave these as intrinsics and just have the intrinsic be super specially typechecked?

cc @eddyb because were were talking about this a couple of hours ago. Basically if we are going to go through all that trouble, we must type check that the indices in the array of constants are in bounds. In particular, that for the one vector case they are in range [0, T::lanes()) and for the two-vectors case in range [0, 2*T::lanes()).

An alternative would be to have the macro in this PR have zero monomorphization time errors. The naive way to do that would be to not error on an index out-of-bounds in trans, and instead, insert a panic. But honestly I prefer the monomorphization-time error to that solution.

@danielrh

Copy link
Copy Markdown

Another way of approaching this is to have the function take in a trait with associated consts. I haven't found a less clunky way of doing it yet, but it could be something like this, where indices are checked at compile time:

structSIMD{pubdata:[i16;8],}macro_rules! check_indices {() => {fn check_indices(){let _test0:[u8;7 - Self::INDEX[0]] = [0;7 - Self::INDEX[0]];let _test1:[u8;7 - Self::INDEX[1]] = [0;7 - Self::INDEX[1]];let _test2:[u8;7 - Self::INDEX[2]] = [0;7 - Self::INDEX[2]];let _test3:[u8;7 - Self::INDEX[3]] = [0;7 - Self::INDEX[3]];let _test4:[u8;7 - Self::INDEX[4]] = [0;7 - Self::INDEX[4]];let _test5:[u8;7 - Self::INDEX[5]] = [0;7 - Self::INDEX[5]];let _test6:[u8;7 - Self::INDEX[6]] = [0;7 - Self::INDEX[6]];let _test7:[u8;7 - Self::INDEX[7]] = [0;7 - Self::INDEX[7]];}}}traitConstIndices{constINDEX:[usize;8];fncheck_indices();}structBackwards{}implConstIndicesforBackwards{constINDEX:[usize;8] = [7,6,5,4,3,2,1,0];check_indices!();}fnshuffle<Indices:ConstIndices>(vv:SIMD,_ind:Indices) -> SIMD{let v = vv.data;SIMD{data:[v[Indices::INDEX[0]],
v[Indices::INDEX[1]],
v[Indices::INDEX[2]],
v[Indices::INDEX[3]],
v[Indices::INDEX[4]],
v[Indices::INDEX[5]],
v[Indices::INDEX[6]],
v[Indices::INDEX[7]]]}}fnmain(){let _result = shuffle(SIMD{data:[2;8]},Backwards{});}

@gnzlbg

gnzlbg commented Apr 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Another way of approaching this is to have the function take in a trait with associated consts.

Note that the number of indices is variable: you can use shuffles to create smaller or larger vectors than the input ones:

// Given:let a:i32x8;let b:i32x8;// All of these work:let c:i32x2 = shuffle!(a, b,[3,15]);let d:i32x4 = shuffle!(a, b,[1,15,3,12]);let e:i32x16 = shuffle!(a, b,[0,1, ...,15]);

IIUC the associated const approach is going to need a little bit more work, since without const generics, the length of the associated const array cannot be generic either.

@danielrh

Copy link
Copy Markdown

Not sure this is still a good idea, but just to throw this out there with existing mechanisms: what if you had a separate function for narrowing or widening a vector that didn't take arguments (either 0 padding it or repeating it, whatever was easiest/fastest)
eg

// Given:let a:i32x8;let b:i32x8;let c:i32x2 = i32x2::prefix_trunc(shuffle!(a, b,[3,15,0,0]));let d:i32x4 = i32x4::prefix_trunc(shuffle!(a, b,[1,15,3,12,0,0,0,0]));let e:i32x16 = shuffle!(i32x16::concat(a, b),[0,1, ...,15]);

and then teach the optimizer to fuse the two shuffles you do internally
that way the type system stays simple... but it is a little more verbose than it could be.

@gnzlbg

gnzlbg commented Apr 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Ideally shuffle would be just a method on vectors with the following signature:

fnshuffle<constN:usize,R>(self,other:Self,constindices:[usize;N]) -> RwhereR:SimdVector<Item=Self::Item,Length=N>{ ...}

There are multiple problems that we currently have to face:

    1. lack of const generics
    1. lack of const function arguments

We can workaround lack of const generics by using a trait on arrays, so we can specify:

fnshuffle<I:Indices,R>(self,other:Self,constindices:I) -> RwhereR:SimdVector<Item=Self::Item,Length=I::Length>{ ...}

We can work around lack of const function arguments by making it a shuffle! macro instead, which means that we loose method position, but otherwise that's not too bad.

We could make it a very special free function (instead of a macro), by implementing it in MIR typeck as @alexcrichton suggested. There we can require that indices is an array of const items, inspect the array values to error if the indices are out-of-bounds at compile-time, etc.

So we would get a magic "function" with this signature instead:

fnshuffle<T:SimdVector,R,/*N is magic*/>(a:T,b:T,/*magically const*/indices:[usize;N]) -> RwhereR:SimdVector<Item=T::Item,Length=N>{ ...}

This PR implements it as a macro in the language, because that's basically the only way we currently have to do this with the available compiler magic, but I agree with @alexcrichton that doing this in MIR typeck is the best path forward. Maybe as the language gets const generics and const function arguments, the shuffle "function" signature can become less and less magical.

FWIW, once you have shuffle, you can implement a.concat(b) on top of it without any magic:

traitConcat:SimdVector{typeResult:SimdVector<Item=Self::Item>;fnconcat(self,other:Self) -> Self::Result;}implConcatforu32x4{typeResult = u32x8;fnconcat(self,other:u32x4) -> u32x8{shuffle!(self, other,[0,1,2,3,4,5,6,7])}}let a:u32x4;let b:u32x4;let c:u32x8 = a.concat(b);

I think that adding concat to std::simd is something worth doing, but I prefer to nail down shuffle first.

@gnzlbg

gnzlbg commented Jun 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton shall I reopen and merge this. In a nutshell, I agree that it would be better to move this macro to rustc, but I don't have the time to do it, and its API is something worth getting experience with in the meantime.

@danielrh

danielrh commented Jun 4, 2018 via email

Copy link
Copy Markdown

@gnzlbggnzlbg reopened this Jun 12, 2018
@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton maybe we could ask feedback for the lib teams on this?

Comment threadcoresimd/ppsv/api/shuffles.rs Outdated
}
};
($vec:expr, [$($l:expr),*]) => {
shuffle!($vec, $vec, [$($l),*])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will evaluate $vec twice.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Amanieu

What's the best way to fix this? Just:

{let v = $vec;shuffle!(v, v, ...)}

?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

match $vec { v => shuffle!(v, v, ...) } is preferred because of let ... in ... semantics, which regular let doesn't have (less relevant here, but all the temporaries in $vec stay alive for the duration of the match).

@Amanieu

Copy link
Copy Markdown
Member

Are there any plans to support single-element vectors (e.g. u64x1)? NEON has such types and LLVM does not use the same codegen as scalar types for these (for integer types, values are kept in SIMD registers rather then being first moved to a general-purpose register).

@Amanieu

Copy link
Copy Markdown
Member

This is somewhat relevant to this issue since we will need to add a simd_shuffle1 intrinsic to support this, and I was wondering if it was worth extending this to the generic API as well.

@gnzlbg

gnzlbg commented Jun 15, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

I am holding this PR until we have an idea about how to resolve: rust-lang/rfcs#2366 (comment)

@gnzlbg

Copy link
Copy Markdown
ContributorAuthor

Superseeded by https://github.com/gnzlbg/ppv

@gnzlbggnzlbg closed this Jul 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gnzlbg@alexcrichton@danielrh@Amanieu@eddyb