[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

[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

[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

[helehex]

@JoeLoser I made it cute again ;]

[helehex]

I was gonna close this, but then I tried fixing it, went a little crazy, and now I'm moving on with my life.

[helehex]

oh wait

[helehex]

Much better :]

[JoeLoser]

!sync

[helehex]

I'd be surprised if this doesn't break everything.. in that case don't worry about it Joe, I'm sure we'll figure out something better.

[martinvuyk]

Hi, a bit late to the party but won't doing something like adding a conditional conformance overload to eq work ? i.e. fn __eq__(self: Scalar[type], rhs: Scalar[type]) -> Bool:

[helehex]

Yeah that might work now, when i tried it before it complained about not conforming in the general case. i'm in bed now, but I'll add it to my todo list to check it out again tomorrow and see if anythings changed. * oh and resolving conflicts

edit x2 i got out of bed to open a pr in sdl-bindings, and then i realized what you mentioned is actually a little different.. i was confusing it with having condition __bool__() conformance like that, hah..

[martinvuyk]

i was confusing it with having condition __bool__() conformance like that, hah..

Well I guess it could be used for that too ? But you wouldn't have the nice and clear error message that the constraint has

But yeah my idea was this:

struct A[size: UInt]:
    var field: Int

    fn __init__(inout self, field: Int):
        self.field = field

    fn __eq__(self: A[1], rhs: A[1]) -> Bool:
        return self.field == rhs.field


fn main():
    print(A[1](1) == A[1](1))
    print(
        A[2](2) == A[2](2)
    )  # invalid call to '__eq__': could not deduce parameter 'size' of parent struct 'A'

This works fine in a separate file, but I don't know if it'd pass all the tests for SIMD.

The only bad thing I see about doing it like this is that the error message is not clear, the user would have to go look at the function signature to understand what is going on.

[helehex]

make sure it works when passing it as an EqualityComparable

[JoeLoser]

!sync

[JoeLoser]

!sync

[helehex]

I removed the string overloads for assert_equal() as well, since those were causing problems

[helehex]

oh good a docstring problem, i wanted to change something anyways :] Oups, it's """Parameters:""", not """Param:"""

[helehex]

!sync