[stdlib] SIMD conformance to EqualityComparable

[helehex]

This allows SIMD to conform to EqualityComparable, without losing any of the original behavior.

It uses the 4th overload resolution rule to give the new methods lower precedence, while still conforming to EqualityComparable

[gabrieldemarmiesse]

I wouldn't be against some more details (in the docs or something) since this seems a bit like black magic and using return types for overload resolution :)
Maybe the docstring would be a good place to explain how to actually use this overload?

[helehex]

Yeah that might be good. I'm not even sure this is something that would get pulled because of that, but figured i'd give it a shot since it's just a few lines and gives expected behavior

[JoeLoser]

This is a pretty neat way to make SIMD conform to EqualityComparable, props to your cleverness!

This seems like something we should do in the short-term until we have conditional conformance and other things. Accepting this PR would unlock a bunch of other future nice things. To start with, we could then push this EqualityComparable requirement into CollectionElement, which would then mean we can now implement __eq__ and __ne__ for collections. Further, it would allow us to simplify things like List.count and List.index which currently use an ad-hoc composite trait to ensure the given type implements __eq__ since that isn’t a requirement of CollectionElement.

I've added this PR to bring up with the team on Monday during our internal design discussions, and I'll circle back after we meet. But this is super cool, thanks for pushing on this!

[JoeLoser]

Suggestion Use assert_true in this line and assert_false in the line below rather than assert_equal with True and False, respectively? The intent will be clearer IMO. Ditto elsewhere in this file where that pattern shows up.

E.g.

assert_true(eq(s1, s1), "eq(s1, s1)")

or even drop eq and friends entirely in favor of:

assert_true(s1 == s1, "s1 == s1")

What do you think? Is your intent of the existence of eq and ne to ensure SIMD conforms to EqualityComparable trait, or something else? If so, it's not needed as the compiler will ensure SIMD conforms to the trait since you explicitly said SIMD shall conform to it in its trait list. If it didn't implement the necessary requirements, mojo would issue a hard error at compile time.

[helehex]

The first should work, but i think the second one might cause issues.
however i should be able to do assert_equal(s1, s1) and assert_not_equal(s1, s2)

The downside of using this trick:
the behavior of != changes slightly, depending if you're in the context of EqualityComparable
(because __bool__ will always use reduce_and), so i was using the ne function to ensure that.

I'll try it out in a lil bit though

[helehex]

sora pointed out that this would not be confluent under generalization, and we agreed that for now, SIMD.__bool__() should be constrained to size=1, so that if the logic of bool(a != b) changed out from under you, it at least gives a compiler error

[helehex]

we also agree that being explicit about reduce_and/reduce_or makes the most sense.
maybe something like specifying parameters on self would be nice here, to see the error in the editor:

fn __bool__(self: SIMD[type, 1]) -> Bool:
    ...

[helehex]

ok this feel like the right way to go

• Added some simd helper functions: _all_equal, _any_equal, _all_not_equal, _any_not_equal.
• Changed every case i could find of implicit reduce functions to be explicit. (tests pass but may have missed some)
• SIMD.__bool__() constrained to size=1
• Removed some things that are unnecessary now, like overloads for assert_equal and assert_not_equal
• Tweaked some tests to make more sense
• Fixed a pre-existing bug with _isclose() while changing to explicit reduce

[soraros]

You might need to coordinate with @leandrolcampos a bit because of #2375.

[helehex]

I'm still not sure this should or will get pulled. It feels good writing simd code like this. You get a compiler error when converting a SIMD with size > 1 to Bool, such as with if statements, but it doesn't show up in the editor. I can see that being confusing. maybe it's still better than what we have currently.

[soraros]

@helehex IMHO, the foot gun mitigation probably already made it worth it.

[helehex]

another trade-off that should be mentioned:
Ideally, everything should be boolable, and this breaks that
but like sora mentioned, when working with simd size > 1, the boolability causes trouble

[JoeLoser]

Thanks for pushing on this! I think this is a general direction we want to go with, but there's a couple things going on in this PR I'd recommend we decompose/split up into separate PRs. Specifically, I'd recommend

1. Constrain __bool__ to ensure size is 1 to avoid this footgun. I'd recommend beefing up the constrained message to be something like

constrained[width == 1, "The truth value of a SIMD vector with more than one element is ambiguous. Use `v.reduce_and()` or `v.reduce_or()`"]()

2. I'd like to see a proposal of just changing SIMD.eq to return a Bool instead of being cute with the overload rules. I think this is a better long-term approach for generic programming and the footgun we have now with SIMD equality. I'd recommend we reference things like Swift and Rust when considering this API change. Take a look at https://github.com/apple/swift-evolution/blob/main/proposals/0229-simd.md#detailed-design and how Rust models its simd_eq and friends here. In addition, I'd recommend we also discuss how we want to model the other comparison operators. This may change in the library once we have extensions in the language too.

3. Split up the constructor removal into standalone PR? I don't think it needs to be coupled with these changes, right?

@abduld I'm also curious to get your general thoughts here as I know I've brought up my concerns with the existing SIMD.eq in the past.

[helehex]

everyone i've talked to likes how simd.eq returns a simd though

[helehex]

I agree we need a proper proposal though.
For now i'll just open a separate pr for constraining simd.__bool__. That will also align better with simd.__int__

[helehex]

removing the bool constructor wasn't necessary, but it helped weed out all the cases of implicit reduce and is also very much related

[helehex]

it finally occurred to me that reduce_and has bitwise ambiguity