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

Make sure we don't miss any other trait methods that people expect to be lanewise instead #257

Closed
10 tasks done
programmerjake opened this issue Mar 8, 2022 · 8 comments
Closed
10 tasks done

Comments

@programmerjake
Copy link
Member

programmerjake commented Mar 8, 2022

we currently have min/max (#247) and clamp (#253) that are missing from Simd for integers, we should go through all the trait methods for std traits implemented by Simd and Mask and ensure that users get the appropriate lanewise version where expected.

incomplete list (edit as needed):

Integers

  • Ord::min
  • Ord::max
  • Ord::clamp
  • PartialEq::eq
  • PartialEq::ne
    ...

Masks

  • Ord::min
  • Ord::max
  • Ord::clamp
  • PartialEq::eq
  • PartialEq::ne
    ...
@scottmcm
Copy link
Member

scottmcm commented Mar 8, 2022

I'll note that LLVM has vector intrinsics for integer min/max now: https://llvm.org/docs/LangRef.html#llvm-smax-intrinsic

@RalfJung
Copy link
Member

Why is Ord::clamp not on the list for masks?

@Lokathor
Copy link
Contributor

masks are effectively a list of bools, doesn't make sense to clamp them really.

@programmerjake
Copy link
Member Author

Why is Ord::clamp not on the list for masks?

because the list is whatever stuff i felt like adding at the time...it still needs to be filled out.

@RalfJung
Copy link
Member

RalfJung commented Mar 10, 2022

masks are effectively a list of bools, doesn't make sense to clamp them really.

That would be an argument for not having m.clamp in the first place. But that is impossible if they implement Ord.
The question is just whether clamp works lexicographically or lanewise.

because the list is whatever stuff i felt like adding at the time...it still needs to be filled out.

Okay. :) Given that it was listed for integers, not having it listed for masks looked like a deliberate decision.

@workingjubilee
Copy link
Member

I made the list formatted nice and I left clamp out because my brain failed to parse what clamp even does on, yes, a "vector of bools".

@RalfJung
Copy link
Member

clamp is defined the moment max and min are defined, so if you can imagine max and min on bools then just derive clamp from that. ;)

fn clamp(self, lo, up) { self.max(lo).min(up) }

I think it would be a mistake to overload max and min but not also clamp, as that would violate the above definition that should always hold.

@calebzulawski
Copy link
Member

This is now done via the SimdPartialOrd, SimdOrd, and SimdPartialEq traits, which are implemented on both regular vectors and masks (added in #265).

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

6 participants