SE-0229 — SIMD Vectors

Evolution Proposal Reviews
1

The review of SE-0229 — SIMD Vectors begins now and runs through October 5, 2018.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager (via email or direct message in the Swift forums).

What goes into a review of a proposal?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.

When reviewing a proposal, here are some questions to consider:

  • What is your evaluation of the proposal?

  • Is the problem being addressed significant enough to warrant a change to Swift?

  • Does this proposal fit well with the feel and direction of Swift?

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Thanks,
Ben Cohen
Review Manager

12 Likes
2

While I am not going to be able to give a formal review of the proposal. I'd like to provide minor feedback.

It is not immediately clear to me that

Float.Vector[2,3,4,8,16] Double.Vector[2,3,4,8]

https://github.com/apple/swift-evolution/blob/master/proposals/0229-simd.md#proposed-solution

...would mean.

Float.Vector2

Float.Vector3

Float.Vector4

Float.Vector8

Float.Vector16

etc.

I know its verbose but I much rather have this spelled out. (I am not commenting on the actual spelling of it, just on how it is written as a shorthand Float.Vector[2,3,4,8,16])

I realized the spelling once I hit the end. ``Float.Vector8 (this proposal)

8 Likes
3

+1
It's a feature that's most likely only beneficial for a minority and therefore not very significant, but it could be a game changer for that small group, without hurting the majority.

I hope there will be better syntax to define such types in the future, but waiting for a major new generics feature would most likely cause a huge delay.
Vector4.Float might be better suited for an hypothetical migration (Vector<4, Float>) than Float.Vector4 - but if we ever allow integer values as generic parameters, we might as well choose Float<4>...

3 Likes
4

+1 to the proposal as it stands.

I appreciate these types being added the the stdlib. I agree with the naming choices. That is all I can say now.

I am very busy these days, but I have tried to follow the discussions but don't have time for a deep study of the proposal or a detailed response.

1 Like
5

@Ben_Cohen Should apple/swift-evolution#916 be merged for this review?

3 Likes
6

Ah, yes it should. Now done.

For anyone wanting to know what changed if they read through the previous version: diff here

2 Likes
7
  • What is your evaluation of the proposal?

Mostly in agreement with the proposal as written, except that I would prefer the SIMD types to be e.g. SIMD4.Int8, and to use the normal +, - and * for element-wise operations.

However, if the type names do not include "SIMD", as in the current proposal, then not using +, - and * for element-wise operations is acceptable.

Furthermore, I think SIMD is sufficiently separated from the rest of the stdlib that it may be beneficial to have it be a core library, instead.

  • Is the problem being addressed significant enough to warrant a change to Swift?
    Yes.

  • Does this proposal fit well with the feel and direction of Swift?
    Predominantly yes.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Read through proposal several times.

1 Like
8

Great proposal! Though I would like to suggest some improvements:

  1. Generalize SIMDVector.init(_ array: [Element]) to SIMDVector.init<S: Sequence>(_ seq: S) where S.Element == Self.Element.

    • This allows more sequences to be used, such as ArraySlice, and lazy collections

    • Then, SIMDVector.random(in:using:) can be more simply implemented as:

      return Self((0..<self.count).lazy.map { _ in
    Element.random(in: range, using: &generator)
})

  • If we're not going to make SIMDVector a sequence, then we should consider other ways to convert it to a sequence. Perhaps a function that returns the elements as [Element], or a customer sequence type that iterates the packed values directly.

  • What is your evaluation of the proposal?

    +1

  • Is the problem being addressed significant enough to warrant a change to Swift?

    +1

  • Does this proposal fit well with the feel and direction of Swift?

    Generally, I think so. Although if we ever plan to support non-type generic parameters, perhaps we might want to spell this like Vector<4, Float>.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

    Proper reading

3 Likes
9

This is great, I'm thrilled this proposal is driving SIMD forward. We need to sort out this area, and Steve is world expert in this area.

Yes.

Generally yes, details below. I would also like to see the design of this get carried forward to the design of Tensor APIs, so this is an important topic to me.

I have used a lot of these, and designed a fair bit of the OpenCL syntax. The systems I am most familiar with are full of a bunch of suboptimal concessions that we are not affected by, I'm glad to see a bigger view on this.

Quite a bit of time, including being involved a bit in the pitch thread and reading the proposal in depth.

Detailed feedback:

  • "Authors: Stephen Canon" -> "Author:". Pet peeve of mine :slight_smile:

  • On general syntax, I prefer Vector8<Float> for the alignment with existing aggregate types in Swift, and that it would put all the Vector stuff in a common type family that is easily reached with code completion. The discussion in the "alternatives considered" section seems to center on implementation concerns while also admitting that they are solvable. I think we should be principally driven by the user model here.

  • The comment above associatedtype Mask seems to imply that the mask would be all ones values, but some low level targets just set the sign bit of values. Does your design work efficiently for both sorts of hardware?

  • Agreed on not having Collection conformance.

  • I'm super thrilled you're using .== for the equality operator.

  • The SIMDIntegerVector specialization should use . prefixes for comparison operators that return Mask, e.g. .<, for the same that == does this.

  • In contrast, I don't think that pointwise multiplication should use a dot prefix, it should just be spelled *, to make it uniform with + and -. This establishes the precedent that all the unary/binary arithmetic operators (and methods like sqrt) operate pointwise when they are (T,T)->T, and it implies we use some other operator (e.g. • or something else) to represent dot product and matmul in higher spaces. Comparisons using . prefix is justified because these things diverge from the normal (T,T)->Bool signature.

  • I don't understand why SIMDMask is defining strict versions of && and ||. this is inconsistent with the rest of the language - they should be & and | operators which are strict.

  • "There is also a set of protocols defining the vector sizes:" - if these are going to be public, then please provide more details in the proposal.

  • Why isn't permute/shuffle described as a core operation on the type in the section above? This proposal seems like it isn't self contained and is leaving out other important behavior of the types.

Thank you again for driving this forward, I'm a huge fan of this work!

-Chris

9 Likes
12

The representation of Masks is deliberately under-specified to allow for such architectures, and also for architectures that use a single bit per lane. Someone would have to write the arch-specific implementation of the comparison operators in builtins for one of these, and possibly update the subscript operation, but everything else should be fine.

Unlike ==, there are no Bool-returning ordered comparisons, so this is less clear-cut. We could certainly do it that way, though.

I agree. That's more of a typo than anything.

The problem with & and | is that their precedence is wrong (because they bind more tightly than comparisons). Consider x >= 0 & x < 16. That's parsed as x >= (0 & x) < 16. Fortunately, this is a syntax error because ComparisonPrecedence doesn't have associativity, so it's less of a footgun than it would otherwise be, but it's still not ideal. I discussed this some in the notes section; we may want to define .&, .|, and .^ for this purpose.

2 Likes
13

+1. Definitely significant enough to warrant the addition. Direction and feel is great. I have done a little SIMD and graphics programming but just at a newbie level.

My main comment is that this clearly comes from a subject matter expert, and I think it's a good idea to go with his recommendations. Where I'd make my own specific comments:

Regarding types -- I prefer an implementation that is straightforward and logical over one that has a prettier form of type names but generic types that aren't truly general.

Regarding operators -- I agree that overloading existing operators should keep the same 'type shape', i.e. (T, T) -> T for e.g. *, + and (T, T) -> Bool for e.g. ==, <. Thus, I'd be in favor of overloading the existing arithmetic operators, and in favor of new operator names for the mask operations (.== seems fine).

Thanks!

2 Likes
14

I haven't been able to devote as much time to this discussion and proposals as with others, and therefore don't feel like I can give an appropriately full review, but I do have a few thoughts--

  • Regarding the mask operators, because they don't short-circuit like '&&' and don't have the same precedence as '&', I agree with the alternative proposed, which is to use new operators such as '.&'.

  • Regarding the naming of types, the proposed option is certainly acceptable, the alternative 'Vector3<Int>' has advantages and disadvantages as already discussed, but the final alternative 'Vector3.Int' suffers from what's in my view a fatal flaw that within any extension of that type it could be referred to as simply 'Int', which is altogether too confusing.

  • Regarding some bikeshedding matters:

    • 'elementBytesSwapped' is difficult for me to parse--maybe it's just me, but I wonder if we could go for something like 'elementwiseByteSwapped' both to avoid the mismatch between "bytes" (plural) used in the proposed name here and "byte" (no 's') in the name of the corresponding property on integers, and so that the word "element" is given some context rather than being squashed next to another noun.
    • 'init(bitMaskFrom:)' uses a nonstandard label; we have the older style of bare nouns being used as labels in initializers, and a newer style with words like 'repeating', but I don't think nouns are ever used like this with a trailing preposition; I think 'init(mask:)' would be sufficient here both for brevity, clarity, and consistency of style.

Overall, I'm very excited that a cross-platform SIMD implementation is coming to Swift.

7 Likes
16

I never used SIMD, but I think the syntax used in standard library should not clash with the other type of Vector, the mathematical one. People will write mathematical vectors, and it would be nice if they used the same operators.

Float.Vector8

I like that we don't use <> here. SIMD vectors aren't really generic, they are a fixed set of types.

var nonzeroBitCount: Self { get }

Negation in names seem weird. Isn't a better name something that we don't have to invert in our mind? onesCount, populationCount or bitsSum?

static func <(lhs: Self, rhs: Self) -> Mask
static func <=(lhs: Self, rhs: Self) -> Mask
static func >(lhs: Self, rhs: Self) -> Mask
static func >=(lhs: Self, rhs: Self) -> Mask

If I saw comparison operator on a Vector I would guess that it is a lexicographical comparison. It's not. I would like for all Mask operators to be spelled with a dot.

Are nonwrapping operators + - and .* missing from the document for SIMDIntegerVector, or will they not be implemented?

static func .*(lhs: Self, rhs: Self) -> Self

I like that elementwise multiplication is spelled with a dot, because it leaves * for vector product

+1

I never had to use SIMD. I don't know.

Yes, except the < <= > >= operators

I have not used SIMD.

I've read the pitch thread and read proposal multiple times.

2 Likes
17

What is your evaluation of the proposal?

I am huge +1 on adding SIMD types to the standard library, and on the design in general.

However, I’m missing a more detailed description of all the types proposed. The proposal only lists the concrete types briefly and describes the most important protocols, saying that the rest is self-explanatory. That makes it hard to understand exactly what is proposed. You either have to read through the PR (which is heavily gybbed), or build a toolchain yourself to try it out.

This feels like a break from the standard process. I can’t think of a single proposal that I haven’t been able to review by reading the text alone. Even large proposals like SE-0166 have included all the code examples necessary to understand exactly what’s proposed.

I wonder if this might be why several people have said that they don’t have the time to review this fully.

Is the problem being addressed significant enough to warrant a change to Swift?

Yes. These are fundamental currency types, and should be included in the standard library.

Does this proposal fit well with the feel and direction of Swift?

Yes, in general. But a few things stick out:

  • The .* spelling for elementwise multiplication feels inconsistent with the other arithmetic operators, especially /. I would have expected it to be just *.

  • Having standard equality and/or comparison operators that don’t return Bool is confusing, as pointed out by many. Changing the elementwise equality operator to .==, but not changing the elementwise comparison operators is even more confusing. Whatever spelling is chosen, both elementwise equality and elementwise comparison operators should use it.

  • The _all() -> Bool and _any() -> Bool methods on SIMDMask feel unswifty to me. Since they just return simple computed values, they should be properties, not methods. And since they are perfectly usable by themselves, and not just as customization points, they shouldn’t be underscored. A better spelling would be var all: Bool and var any: Bool.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I’ve done graphics programming in shaders (Cg/HLSL) and Unity (C#), and some in Swift using structs and simd.h. Compared to those, a few things stand out as missing without a clear motivation:

  • What is the motivation behind not including standard property names x, y, z and w for 2, 3 and 4-component vectors?

  • What is the motivation behind including lowHalf, highHalf, evenHalf, oddHalf swizzles, but not xy, xz, yz, etc?

  • What is the motivation behind including square root but not geometric operations like dot product, cross product, magnitude, magnitude squared, etc? Is the user supposed to write those themselves, or are they coming in a later proposal? Or are they at another "level" than these types?

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Participated in the pitch thread, read the proposal, dug through the PR.

8 Likes
18

They are provided.

(swift) let a = Float.Vector4(1,2,3,4)
// a : Float.Vector4 = Vector4(1.0, 2.0, 3.0, 4.0)
(swift) a.y
// r0 : Float = 2.0

For four-element vectors, there are 340 different swizzles. low, high, even, odd plus the individual elements provide the most common ~50% of usage, with the other 50% being more or less evenly distributed among the remaining 332 values. It may also make sense to add inits that zero-extend and truncate vectors.

The remaining general case is covered by:

(swift) Float.Vector4(gathering: a, at: [2,1,3,0])
// r1 : Float.Vector4 = Vector4(3.0, 2.0, 4.0, 1.0)

If there are a few specific swizzles needed frequently in your code, I would define them in an extension using the init above.

Coming in a later proposal. This proposal is about the fundamental types and operators. I expect multiple proposals over the next couple years to build on this.

6 Likes
19

This is an excellent point, which I totally glossed over in my writeup. Thanks for raising it.

20

Great! I must have missed it going through the code.

Nice. Extensions based on that should definitely cover all use cases. Despite reading the proposal several times, I glossed over this. Maybe because the signature didn't include the word "swizzle" (which might not be enough of a term-of-art to use here, but is what I've always called this). A doc-comment would probably have helped too.

Also, shouldn't it be at indices rather than at index?

Great! That could probably be clarified in the proposal.

2 Likes
21

Review manager moderation comment

I've removed a handful of posts that were just links to documentation of or articles about SIMD types in other languages, without any additional text.

It is definitely valuable in proposals and during reviews to refer to prior art in other languages – perhaps particularly in this case.

However, these posts were just links without observations, conclusions or recommendations. Any posts to the review thread referring to other languages should contribute something about the pros/cons of those other languages, and some commentary of how this proposal differs in good or bad ways, rather than just providing links.

10 Likes
22

Awesome, makes perfect sense. Please add a comment to the Masks requirement that indicates that the type is target dependent and may very well have different size across targets. Are there any other observable differences in behavior that these types will have, or is that prevented by them presenting a narrow api interface? For example, do you intend the concrete type for a mask to be Vector4 (in which you can fully observe the representation) or do you intend it to be a narrow thing like VectorMask4 which only has logical operators and can be subscripted?

We should definitely have .!= and bool returning != at least :-)

Given this direction, I think it would be very inconsistent to have < return a mask but have == return a bool. I think we should standardize on .< (or <. exactly spelling not important) for pointwise comparisons. Among other things this keeps < as having a (T,T)->Bool signature, which is important for user model, type checker performance, etc as discussed on the pitch thread.

Oh ok, it seems to be causing some confusion in the review, so I strongly recommend patching the proposal in place ASAP to fix this typo (also fix "Authors" ;-)

Oh I see, I'd really prefer that we not make && and || strict though. I agree that defining a .& and .|, and .^ operator would solve this. I also agree that the footgun probability of defining & is tiny.

Between the choices of & and .&, the argument to add new .& .&& sorts of operators really seems to come down to the question of how important is it to reduce the need for parens. IMO, this isn't important enough to justify adding the new operators, so I'd lean towards just using & and | and ^ and being done with it. This preserves a nice symmetry with the other binops as well.

-Chris

2 Likes
23

They do conform to the Vector${N} protocol, but that really doesn't add much. Their API surface is still quite narrow, and does not make any claims about their memory layout. The interface is very much "these are a vector of Bool", and does not traffic in the representation at all.

I'll note that if you want to map Masks into a defined layout, you can initialize an integer vector from them. We might also define a load/store from "compressed" bitmask form for folks who want to serialize them, I guess.

I'll be posting an update shortly reflecting the other notes you and others gave.

1 Like