Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Are rotate_left/right/swap_bytes methods possible? #419

Closed
TheIronBorn opened this issue Apr 10, 2018 · 16 comments
Closed

Are rotate_left/right/swap_bytes methods possible? #419

TheIronBorn opened this issue Apr 10, 2018 · 16 comments

Comments

@TheIronBorn
Copy link
Contributor

TheIronBorn commented Apr 10, 2018

I'm curious what it would take to get functionality like integer primitives' methods.

@TheIronBorn TheIronBorn changed the title Are rotate_left/right functions possible? Are rotate_left/right methods possible? Apr 10, 2018
@TheIronBorn
Copy link
Contributor Author

I'm also curious about swap_bytes and others. How many of the missing primitive methods are possible to implement here?

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 10, 2018

Sine one can easily perform these using shuffles, I think the question is rather whether it is worth it to implement these. Is there an ISA that supports them?


EDIT: FWIW horizontal rotate_left/right would need to rotate bytes and not bits, so that might be an issue. Vertical rotates should be implementable on top of the shift operators + splat, and the bitwise operators. I don't know which names to use to differentiate these though, maybe rotate_left for vertical ops and rotate_elements_left for horizontal ops ?

For swap bytes both vertical and horizontal versions can use shuffles AFAICT.

@TheIronBorn
Copy link
Contributor Author

TheIronBorn commented Apr 10, 2018 via email

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 10, 2018

To be clear, I want to rotate each lane

I see. In the meantime you can manually implement rotates using bitwise operators, shifts, and splat:

#![feature(stdsimd)]
use std::simd::*;
fn rotate_left(x: u32x4, n: u32) -> u32x4 {
    let n = n % 32;
    (x << n) | (x >> (32 - n) % 32)
}

(note that vector << n currently means vector << splat(n), but this behavior will be deprecated soon and one will need to explicitly use splat in the near future)

Not shuffle the lanes around.

The problem is that "rotate left" is both an operation on the primitive type (e.g. u32 above), and an operation on a sequence of element types (e.g. &mut [u32]). Both operations are equally reasonable things to want to do on vector types so we can't really give them the same names. Until now the naming schemes used try to reduce the friction of vertical operations, so I think using rotate_{left,right} for vertical operations and something else for the horizontal operations makes sense (e.g. rotate_elements_{left,right}).

For swap_bytes I don't know which names would be good.

I'm curious what it would take to get functionality like integer primitives' methods.

Somebody sending a PR with a tested implementation might just be enough. From there to having these land on stable Rust, a mini-FCP might be enough as well.

@TheIronBorn
Copy link
Contributor Author

TheIronBorn commented Jun 25, 2018

vertical rotates pull request here #496

@TheIronBorn
Copy link
Contributor Author

We need swap_bytes_horizontally for rand, should I make a PR here or implement it in rand? pitdicker/rand#1

@TheIronBorn TheIronBorn changed the title Are rotate_left/right methods possible? Are rotate_left/right/swap_bytes methods possible? Jun 29, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 29, 2018

You can send a PR. I think we can just call it swap_bytes for the horizontal reduction. If we ever want to add the vertical operation we can call it swap_element_bytes to denote that it swaps the bytes of each element.

Just keep in mind that llvm.bswap only works for integers, not vectors, so you are going to have to cast the 256-bit and 512-bit vectors to i256 and i512 somehow..

@TheIronBorn
Copy link
Contributor Author

Oh I was thinking to use shuffles. Would llvm.bswap be better?

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 29, 2018

Indeed, shuffles should definitely work. I don't know if llvm.bswap would be better (maybe @rkruppe knows). For the portable vector types, you can define a trait in the codegen module and implementing for each type using shuffles, and then the macro just calls the trait (e.g. check how it works for sqrt). We can then choose to use shuffles or bswap for each type independently.

@hanna-kruppe
Copy link

I have little context but ISTM that element-wise bswap is quite a different operation from "interpret the vector as a huge integer and bswap that". But even if I'm mistaken about that, huge integer types like i256 and i512 (and even i128 to a lesser degree) have very spotty support in most backends even functionality-wise, let alone in terms of performance optimizations. Shuffles OTOH sound great. Except:

Just keep in mind that llvm.bswap only works for integers, not vectors, so you are going to have to cast the 256-bit and 512-bit vectors to i256 and i512 somehow..

Where'd you get that idea? https://godbolt.org/g/NG6Gxu

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 29, 2018 via email

@TheIronBorn
Copy link
Contributor Author

Swap bytes PR #509

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 30, 2018

@rkruppe https://llvm.org/docs/LangRef.html#llvm-bswap-intrinsics doesn't mention vectors, only integers :/

But if bswap works for vectors we should just try it out and choose what works best (and report llvm bugs for the other).

@hanna-kruppe
Copy link

hanna-kruppe commented Jun 30, 2018

That's curious. But I'm inclined to say it's a docs bug (that someone could report to get clarity) since most similar intrinsics can be applied to vectors "by default" and vector-bswap even has target-independent tests.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 2, 2018

I've reported the bug: https://bugs.llvm.org/show_bug.cgi?id=38012

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 19, 2018

This is fixed in https://github.com/gnzlbg/packed_simd

@gnzlbg gnzlbg closed this as completed Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants