Review of V0.8.4 (part 1)

[allenwb]

Allen Wirfs-Brock 9/18/2015

(ok, github doesn't like my markdown (which worked fine in my editor. It's more important to get this posted than to debug it. hate markdown!!)

Overall API design concerns

I still have a number of significant issues with the overall design of this API.

• As currently specified this proposal introduces over 500 distinct intrinsic functions (methods) into every Realm (eg, iframe). While I haven't done the count, I'm pretty sure that is more than all the rest of ES. Each of these functions must have a distinct per realm identity and (at least conceptually) must be exist when code is first evaluated within the realm. It seems very likely that will have a real impact on Realm startup times, even if an implementation uses lazy instantiation techniques.
• We should not be introducing such a tax for such a specialized and infrequently used feature. That are several ways to address this issue:
  • Make SIMD a built-in module. Then the overhead would only be accrued by realms that explicitly import the module. A module is the right place to place such specialized functionality.
    • Making this a standard built-in module should not be blocked by the lack of a complete module loader spec. TC39 don't need such a spec. in order to start defining built-in modules all we need to do is define the naming convention used for them.
  • Organize the SIMD constructors into a hierarchy with shared inherited methods. Only define most SIMD functions once rather than have one distinct for each distinct SIMD constructor. This will reduce the total number of per realm methods my almost a factor of 10. This technique has already been used by ES6 for Typed Arrays to minimize the number of redundant per realm functions.
    • There should no be performance concerns about such a hierarchical structuring. High perf JS implementation already need to know how to optimize prototype lookup and specialize inherited methods.
  • Replace the 98 cross-product derived methods (SIMDConstructor.fromTIMDBits and SIMDConstructor.fromTIMD) with two (preferably inheritable) methods from(value) and fromBits(typedArray)
• The SIMD operations really should be instance methods, just like they are in the original Dart design.
  • As noted above, optimizing JS engines know how to deal with this value based specialization so performance considerations does not seem like very compelling argument for such a major deviation from the OO norm of associating polymorphic instance specific operations as instance methods rather than class (ie, static) methods.

Section 1 (Subclause 4.3)

Subclause 4.3 historically hasn't tried to list every term used within the specification. Instead, most terminology is defined in the subclauses that introduce the terminology. Lacking an editorial effort to turn 4.3 into a comprehensive glossary, it would probably be best to not add 11 new SIMD related terms within 4.3. Instead, I suggest that you just add three terms that parallel the legacy primitive types:

SIMD value

member of one of the SIMD types

SIMD types

family of types (Float32x4, Int8x16, etc) whose values are homogeneously vectors of numeric or Boolean values.

SIMD objects

members of the Object type that are instances of one of the standard built-in SIMD constructors.

Other terms that are need should be either defined as part of subclause 6.1 (ecma-262 numbering) or wherever the "6 SIMD" section of this draft end up within ecma-262

Section 2 (clause 6)

The SIMD types and values are ECMAScript Language Types and hence, they must be defined as part of subclause 6.1. They should not be in a separate subclause at the same level as the existing 6.1 and 6.2. The SIMD types need to be mentioned in the first paragraph of 6.1 and a new subclass (6.1.8?) ) should be added corresponding to 2.2 in the draft. However, I would structure it considerably differently (use 6.1.4, 6.1.5, and 6.2.2 as models):

• Some of the material currently in section 6 of the draft should move here
• Start with an prose overview:
  • "An SIMD value is a homogeneous vector of ..."
• Then define SIMD values in terms of a single SIMD primitive type rather than as ten distinct SIMD Type primitive types.
  • Use a table to define the fields that make up a SIMD value
  • The specified state of a SIMD primitive value should be sufficient to describe its semantics, without reference to specific SIMD constructors or wrapper objects (note that SIMD values might be used in a Realm other than where it was created). That's why TypedArray instances each have a [[TypedArrayName]] slot that serves as a key into Table 49. My preference would be to have a [[SIMDSubtype]] field in each SIMD value whose value indexed a similar table. However, having a [[SIMDTypeDescriptor]] field in each SIMD value that references a shared descriptor that includes a field that identifiers the SIMD subtype would also be fine. The main problem with using a descriptor is that it is suggestive of associating runtime metadata with each SIMD value when that is not actually necessary.
  • It appears that most of the [[propName]] "properties" listed in section 6 should be either fields of SIMD values or fields of the shared descriptor that is referenced from such SIMD values.
  • Eliminate your section 2.2 where you describe the ten individual SIMD primitive types using boilerplate language for each. Instead simply have a table, with columns for the type name, element type (as in Table 49), vector length. This is independent of you decision to use an explicit descriptor in your algorithms. A table makes it much to see similarities and difference than having to parse repeated boiler plate language.

Switching to a single SIMD primitive value type also eliminates the need for italic SIMD convention in algorithms. Instead your algorithm can simply access and dispatch on the value of the SIMD primtive value field that identifies its specific subtype.

Section 2.1.1.1 (Subclause 6.1.7.4)

I think we should try to more closely match the conventions established (via Typed Arrays native errors) when extending Table 7 with SIMD intrinsic.

One of those conventions, is to explicitly have a separate table row for each Global intrinsic rather than using a single row to collectively describe sets of intrinsics such as TypedArray or NativeError . Also intrinsic are that are property of globally named objects are typically named using an underscore after the global component of the name. That suggests for the SIMD constructrors we should have rows (lexical-graphically ordered) for: %SIMD_Bool16x8%, %SIMD_Bool32x4%, %SIMD_Bool8x16%, %SIMD_Float32x4%, %SIMD_Int16x8%, %SIMD_Int32x4%, %SIMD_Int8x16%, %SIMD_UInt32x4%, %SIMD_Int16x8%, %SIMD_UInt8x16%.

This proposal also suggests defined named intrinsics for the built-in functions Array.prototype.joint, Math.abs, Math.fround, Math.max, Math.min, Math.sqrt, Math.imul. So that they maybe invoked by algorithms defined in this proposal. ECMA-262 and ECMA-402 have generally tried to avoid such direct invocations of specific intrinsic functions because because they can be problematic in some situations (for example, they can have realm locality) issues. For each of these I will suggest (in later sections of this review) alternatives that don't require the introduction of additional named intrinsics.

Section 3 (clause 7)

Section 3.1.2 (Subclause 7.1.2) ToBoolean

I'd leave out the NOTE as no comparable explanation is provided for the other argument types. It's find to have in the proposal labeled as "Rationale", but such rationales are not placed in the actual spec. document.

Section 3.1.4 (Subclause 7.1.12) ToString

As a general principal, the definition of primitive operations upon primitive values should only invoke other primitive operations. . This definition of ToString for SIMD values violates that principal by invoking a built-in method (which is not a primitive operations). [Rationale: such a invocation is an up-level dependency within the layered architecture of the specification. This is a potential hazard to future spec. revisions. As changing the definition of a method in the ES library library layer shouldn't implicitly cause a change in the semantics of a primitive value and its associated primitive operations.]

Instead, similarly to the handling of Number values, an algorithm for an abstraction operation (based upon primitive operation) that converts primtive SIMD values to primitive string values should be defined in anew subclause of 7.1.12. such an algorithm is not hard to write. For example, steps 4-9 of 19.2.1.1.1 is an algorithm that converts a List of ECMAScript values to a comma separated string. This can be used as a model for a SIMD ToString algorithm.

The following comments related to the algorithm currently given in section 3.1.4 and may or may not be applicable to the new algorithm I suggested above:

• Step 1: "...an Array containing..." is not a precise enough description if you were actually creating an Array object. Instead if you needed to create an Array you should use CreateArrayFromList where the list is the [[SIMDElements]] field of the SIMD value.
• Step 1: ...except that the elements of a [[SIMDElements]] list are not ECMAScript language values (eg, Number or Boolean) and hence can't be directly stored as the values of ECMAScript Array elements. Some where you need to define how each of the possible SIMD element values (eg,Float32, Int16, Bool8, etc. ) convert to/from ECMAScript language values.

Section 3.1.5 (Subclause 7.1.13) ToObject

This should probably use a new abstract operation that is more explicit about how a SIMD wrapper object is created for a SIMD primitive value.

Section 3.3 (Subclause 7.2.9) SameValue

If a single SIMD primitive type is used, as suggested above, then step 10 can be rewritten as:

10.  If Type(x) is SIMD, then  
  a. If x.[[SIMDSubtype]] is different from  y.[[SIMDSubtype]], return false.
  b. For i from 0 to x.[[SIMDDescriptor]].[[VectorLength]] - 1, do
    i. If SameValue(SIMDExtractLane(x,i), SIMDExtractLane(y,i)) is false, return false.
    ii.Return true.

Section 3.4 (Subclause 7.2.10) SameValueZero

Do you really want to use SameValueZero to compare the extra vector elements. The only place it would make a difference (in the current ES6 spec) is when using a SIMD value is a Map key or Set element. Do you really want:

 let s = new Set;
 s.add(Float32x4(0,0,0,0);
 console.log(s.has(Float32x4(0, -0, 0, -0)));   //do you want this to display true??

It isn't clear that the rationale for why +0 and -0 should be equivalent when used as Map/set keys also applies to the elements of SIMD vectors.

Does SIMD hardward generate -0 values for vector elements?

Section 3.5 (Subclause 7.2.11) Abstract Relational Comparison

Symbols also throw a TypeError when ToNumber or ToString gets applied to them. I'd either eliminate the NOTE or generalize it so it isn't only about SIMD values.

Section 3.7 (Subclause 7.2.13) Strict Equality Comparison

So, what is the value equality semantics you intend for SIMD vectors that have NaN element values. Do you really intend that Float32x4(NaN,0,0,0)===Float32x4(NaN,0,0,0) is false
and Float32x4(0,0,0,0)===Float32x4(-0,0,0,0) is true?

Section 6 SIMD

I think API design considerations mentioned at the top of this review would have a great impact upon this sections. Below, I'll comment of the section, as written.

I suggest that the top level of this section be patterned more exactly after 20.3 The Math Object. In other words just describe the %SIMD% intrinsic object. don't talk about the the vrious SIMD properties (ie, constructor here). Also the material about SIMD primitive values should be in clause 6.

Re: Note 1. Does this actually occur in this spec? I couldn't find and example. It would be cleaner if such lists were never modified... I guess it happens in SIMDcreate. Assuming that is the only place, that's where the note should be.

Note 2: The use of fields suggest that they need to be present as part of the runtime value. A table is better because it does not carry that implication.

Last paragraph

• This whole paragraph belongs somewhere else. The term "wrapper" isn't
  used in the ES6 spec when describing such objects. Instead, they are
  called "Number objects", "String objects" etc. You should follow
  that same pattern.
  "[[Prototype]] is a SIMDConstructor" can't be correct. It must be an appropriate prototype object.
  ToObject is not the only way that SIMD objects are created. Another way is using the new operator on one of the SIMD constructors.

Section 6.1 Internal algorithm

You might consider moving these into the definition of SIMD values in clause 6, but they are probably ok as a subclause here.

Section 6.1.1 SIMDCreate

• "fields' is not a very descriptive parameter name. Consider something like "vectorElements" instead.
• You need an assertion about the "descriptor" parameter.
• The assertion about "fields" is probably not adequate. You don't actually say it is a List. Besides the length, what else is required of its elements. Are they all primitive values of the type and range supported by the SIMD primitive value implied by the "descriptor"?
• You have a value domain issue with [[CastNumber]]. In most cases its value is an ECMAScript abstract operation (for example, ToInt32, ToBoolean) but for Float32x4 is is %Math_fround% which is a built-in function. Functions and abstract operations are not the same kind of entities and are can not be invoked interchangeably. The easiest fix is probably to define ToFloat32 as an abstract operation.
• Also [[CastNumber]] is a misnomer in that you permit ECMAScript Boolean values to be vector elements and Booleans are not Numbers in ES.
• If you MaybeFlushDemormal when creating the vector elements you don't have to do it when you retrieve them. that will simplify other algorithms.

Section 6.1.2 SIMDToLane

• the NOTE (and similar notes elsewhere) is unnecessary. Implementations can always to such things.
• there should be some sort of assertion about the the type and range of max
• Don't use ToInt32 to convert lane values to positive integers. In ES6 we moved away from this because:
  • ToInt32 does modulo arithmetic on the value, hence large integer values will wrap
  • It converts Infinity to 0.
• Instead we generally use ToLength when we want to cast to a non-negative precise integer value. I'd replace step 3 with:

 - Let index be ToLength(lane).
 - ReturnIfAbrupt(index).
 - If SameValueZero(index, ToNumber(lane)) is false or index ≥ max, throw a RangeError exception.
 - Return index.

Section 6.1.4 SIMDReplaceLane

this abstract operation appears to be only called from one place so you might consider inlining it at it's usage site.

Section 6.1.5 MaybeFlushDenormal

• You might consider a type guard in case n is a Boolean value.
• the term "subnormal floating point value number" is not used else where in the spec. Try to minic the language used in 20.1.2.9.
• Because n can be an abrupt completion value, I would start both alternatives with the step:

  ReturnIfAbrupt(n).
  

Section 6.1.6 SIMDBinaryOp

• You have a domain error for op similar to what was discussed for [[CastNumber]] in SIMDCreate. This one is actually worse as there are three different types of incompatible things passed as arguments for this parameter: built-in function objects, abstract operations, mathematical operations (as defined in 5.2. Plus some arguments use higher order abstract operations (SubSaturate, etc) and looks like they have has errors. It's probably best to always pass an abstract operation and if necessary define additional abstract operations to wrap the application of mathematical operations and built-in function invocations.
• In step 1 say: "a.[[SIMDTypeDescriptor]] and b.[[SIMDTypeDescriptor]] are the same descriptor"
• As noted under SIMDCreate, you shouldn't have to apply MaybeFlushDenormal to the a and b element values because they should have already been added using MaybeFlushDenormal.

Section 6.1.7 SIMDUnaryOp

• domain problems with op similar to those noted for SIMDBinaryOp
• I don't understand the need to the flushDenormal parameter. It is only false when op is unary minus or the abs function. Does some SIMD hardware treat demoralized values differently for those operations?. It seems like that parameter could be eliminated but maybe I'm missing something.

Section 6.1.8 SIMDScalarOp

• make sure op is always an abstract operation
• Should need to MaybeFlushDenormal for elements of a.

Section 6.1.9/10 SIMDLoad/SIMDStore (also 6.2.51 NOTE)

• I strongly recommend passing Data Blocks around in this manner. This was never intended in the original spec. design and it requires significant duplication of spec. material from the Integer Indexed Exotic Objects and Abstract Operations For ArrayBuffers portion of the specification. Such duplication is highly error prone and complicates future spec. maintenance. For example, I see that you are not adequately checking for detached of Array Buffers.
• It appears to me, that all places where you do this could be recast to do indexed access to Typed Arrays (possibly directly using IntegerIndexElementGet and IntegerIndexedElementSet. If those prove to be inadequate use SetvalueInBuffer and GetValueFromBuffer

Section 6.2 SIMDConstructor

For consistency (with the Math object organization) this section should probably be titled "Constructor Properties of the SIMD object" and have a subsection for each of the actual constructors.

A constructor(s) and the static methods of that constructor should not be listed at the same list level. See for example 6.2..1 and 6.2.2.

The same subclause level should not contain both static methods of constructors and abstract operations used by those methods. Either list all of the abstract operations in a separate subclause or place the abstract operation in a subclause of the the method that it is most closely associated with (See 21.1.3.14 for an example of this style.

Bool SIMD types issues with many of the static methods

The Bool SIMD types are specified such that there vector elements are ECMAScript Boolean values because their [[CastNumber]], which is used when populating the vector, is ToBoolean.

Many of the SIMD static methods (for example add, division) are not restricted from use upon Bool vector values. This means that the provided algorithms try to apply mathematical operator such as + to ECMAScript Boolean values. Such values out not in the domain sets of those operations.

Do you really want to perform arithmetic on Boolean values? If so, you need to specify the semantics of all of the operators that are applied to them.

Sections 6.2.5, 6.2.5, etc.

• "This definition uses the refers to + as defined by..." should probably be ""This definition uses + the refers to the abstact operation defined by...", etc.

Sections 6.2.6 mul

• You can't invoke the built-in function %Math_imul% as if it was an abstract operation

Sections 6.2.6 div

• Aren't there any special semantics for integer divisions??

Sections 6.2.11, 6.2.12, maxNum/minNum

• What's the use case for doring max/min that ignores NaNs? Is it common enough to justify including it in this API
• Is it intentional that if both n,m are NaNs that the result is NaN?

Sections 6.2.20, abs

• Why can't this be applied to integer vectors?

Sections 6.2.21, and, etc.

• Why can't just reference 12.11 to explain bitwise logical operations because the primary bitwise algorithm in 12.11.3 is a syntactic evaluation routine. The a and b are not parse trees, so you can't apply 12.11.3 to them.
• Does SIMD hardware actually support bit-wise operations on Float32 vectors and use the same semantic as ES??

MORE TO COME

I'm not done commenting on all of the static methods, but I'm pointing this now to give it visibility before the upcoming TC39 meeting. I'll add additional comments in a followup to this issue.

[littledan]

Thanks for this detailed feedback. I'll work on changes for these errors and clean up the spec generally.

Two very substantial things you raise are right upfront in your comment:

• Putting SIMD on a primordial object, rather than a builtin module. This is an interesting idea, but we don't currently have any builtin modules in ES yet. SIMD.js seems on the verge of shipping in multiple browsers in the first half of next year, and we'd like to avoid blocking on modules. Browsers all seem to have some form of lazy loading for at least DOM content, and some also have it for some ES builtins, so I am not sure if modules are necessary for getting good performance. Let's talk about this in committee, but it seems far from certain to me that it will be a straightforward and fast to add a concept of builtin modules and implement it in browsers. By the way, I think you are overestimating the number of SIMD functions--I believe there are actually a little under 300 static functions, once it is clarified which ones exist (I have updated the spec to improve this, as you pointed out).
• Making SIMD methods, rather than static functions. It seems really hard for me to imagine how this can be squared with good performance. For one, a major use case of SIMD.js is asm.js, which can't deal with calls to objects yet, or use type feedback, or deal with polymorphic functions/methods. But beyond that, in ES implementations like V8, it would give us very poor performance if a function with the same identity is expected to operate on multiple types. We don't specialize inherited methods multiple times. While you point to TypedArrays as an example of this pattern, I haven't figured out how to implement that in a spec-compliant way in V8 without significant VM changes or performance degradation (unless it's just switching between a number of separately generated method bodies for each type, but then what was the point of merging them? We've lost most of the code size reduction benefit, and still added an extra conditional). I'd rather not introduce new cases where there is a tradeoff between performance and correctness if it can be avoided.

If these concerns come back to a performance concern about realm creation time, let's see how implementations get by like this, and we can think about revising it in Stage 4, being the time when we can still make revisions based on implementation feedback.

[littledan]

About the various equality comparisons: I don't think any of the comparison functions will be all that useful for SIMD.js users. I included them with the current definitions out of a desire for consistency with the rest of the ES spec. The SIMD.js spec is written with an eye to generalization into value types (based on the good suggestion of the committee). I thought that, for value types, it would make sense for equality operations to recurse down into fields for deep structural equality.

Are you suggesting that === be defined in terms of calling SameValue on the individual elements of the vector, under the general theory that it would be better to use SameValue everywhere and we're stuck with this legacy === semantics for existing things? I would be OK with that; I was trying to be more conservative initially.

[littledan]

I spoke with some implementors about the change in equality semantics of === to be more like SameValue. The feedback so far is that it would be more difficult to implement this behavior with good performance, and they would prefer to keep the current semantics. Is there any particular problem caused by the semantics in the draft spec?

[littledan]

I've updated the draft spec based on the feedback. I'm not sure if all
of the changes here are feasible, as I've mentioned earlier in this
thread, but here I'll respond line-by-line as to the changes, whether
I have made them or not.

Allen Wirfs-Brock 9/18/2015

(ok, github doesn't like my markdown (which worked fine in my editor. It's more important to get this posted than to debug it. hate markdown!!)

Overall API design concerns

I still have a number of significant issues with the overall design of this API.

As currently specified this proposal introduces over 500 distinct intrinsic functions (methods) into every Realm (eg, iframe). While I haven't done the count, I'm pretty sure that is more than all the rest of ES. Each of these functions must have a distinct per realm identity and (at least conceptually) must be exist when code is first evaluated within the realm. It seems very likely that will have a real impact on Realm startup times, even if an implementation uses lazy instantiation techniques.
We should not be introducing such a tax for such a specialized and infrequently used feature. That are several ways to address this issue:
Make SIMD a built-in module. Then the overhead would only be accrued by realms that explicitly import the module. A module is the right place to place such specialized functionality.

Making this a standard built-in module should not be blocked by the lack of a complete module loader spec. TC39 don't need such a spec. in order to start defining built-in modules all we need to do is define the naming convention used for them.

We could define them, but I'm not sure about a couple things:

• Are we sure that we won't overlap/conflict with what a future loader
  spec will specify?
• How can these modules be used on the web? The <script
  type=module> mode has not yet been standardized or implemented in
  browsers.

  Organize the SIMD constructors into a hierarchy with shared inherited methods. Only define most SIMD functions once rather than have one distinct for each distinct SIMD constructor. This will reduce the total number of per realm methods my almost a factor of 10. This technique has already been used by ES6 for Typed Arrays to minimize the number of redundant per realm functions.

  There should no be performance concerns about such a hierarchical structuring. High perf JS implementation already need to know how to optimize prototype lookup and specialize inherited methods.

  Replace the 98 cross-product derived methods (SIMDConstructor.fromTIMDBits and SIMDConstructor.fromTIMD) with two (preferably inheritable) methods from(value) and fromBits(typedArray)
  The SIMD operations really should be instance methods, just like they are in the original Dart design.

  As noted above, optimizing JS engines know how to deal with this value based specialization so performance considerations does not seem like very compelling argument for such a major deviation from the OO norm of associating polymorphic instance specific operations as instance methods rather than class (ie, static) methods.

This will make it impossible to use in asm.js, and even outside of
asm.js, V8 doesn't do well on optimizing patterns like this.

The class hierarchy also doesn't make sense as some operations are
defined on some types and some on others, and this doesn't follow a
clear nesting pattern.

##Section 1 (Subclause 4.3)##

Subclause 4.3 historically hasn't tried to list every term used within the specification. Instead, most terminology is defined in the subclauses that introduce the terminology. Lacking an editorial effort to turn 4.3 into a comprehensive glossary, it would probably be best to not added 11 new SIMD related terms within 4.3. Instead, I suggest that you just add three terms that parallel the legacy primitive types:
####SIMD value
member of one of the SIMD types
####SIMD types
family of types (Float32x4, Int8x16, etc) whose values are homogeneously vectors of numeric or Boolean values.
####SIMD objects
members of the Object type that are instances of one of the standard built-in SIMD constructors.

Other terms that are need should be either defined as part of subclause 6.1 (ecma-262 numbering) or wherever the "6 SIMD" section of this draft end up within ecma-262

I added these terms in response to other reviewers who were having
trouble understanding the implicit definitions, so I don't want to
remove them. I don't see terms defined in other subsections of the
ES2015 spec.

##Section 2 (clause 6)

The SIMD types and values are ECMAScript Language Types and hence, they must be defined as part of subclause 2.1. They should not be in a separate subclause at the same level as the existing 6.1 and 6.2. The SIMD types need to be mentioned in the first paragraph of 6.1 and a new subclass (6.1.8?) ) should be added corresponding to 2.2 in the draft.

Done

However, I would structure it considerably differently (use 6.1.4, 6.1.5, and 6.2.2 as models):

Some of the material currently in section 6 of the draft should move here
Start with an prose overview:

"An SIMD value is a homogeneous vector of ..."

Done

Then define SIMD values in terms of a single SIMD primitive type rather than as ten distinct SIMD Type primitive types.

Use a table to define the fields that make up a SIMD value
The specified state of a SIMD primitive value should be sufficient to describe its semantics, without reference to specific SIMD constructors or wrapper objects (note that SIMD values might be used in a Realm other than where it was created). That's why TypedArray instances each have a [[TypedArrayName]] slot that serves as a key into Table 49. My preference would be to have a [[SIMDSubtype]] field in each SIMD value whose value indexed a similar table. However, having a [[SIMDTypeDescriptor]] field in each SIMD value that references a shared descriptor that includes a field that identifiers the SIMD subtype would also be fine. The main problem with using a descriptor is that it is suggestive of associating runtime metadata with each SIMD value when that is not actually necessary.
It appears that most of the [[propName]] "properties" listed in section 6 should be either fields of SIMD values or fields of the shared descriptor that is referenced from such SIMD values.
Eliminate your section 2.2 where you describe the ten individual SIMD primitive types using boilerplate language for each. Instead simply have a table, with columns for the type name, element type (as in Table 49), vector length. This is independent of you decision to use an explicit descriptor in your algorithms. A table makes it much to see similarities and difference than having to parse repeated boiler plate language.

Switching to a single SIMD primitive value type also eliminates the need for italic SIMD convention in algorithms. Instead your algorithm can simply access and dispatch on the value of the SIMD primtive value field that identifies its specific subtype.

I don't want to write a specification which would dispatch from a
single function on multiple SIMD types at runtime. It would be
difficult to implement that efficiently. I'll look into whether it
would make sense to rephrase things in terms of a subtype, but I think
such a subtype relation should be purely spec-internal.

Section 2.1.1.1 (Subclause 6.1.7.4)

I think we should try to more closely match the conventions established (via Typed Arrays native errors) when extending Table 7 with SIMD intrinsic.

One of those conventions, is to explicitly have a separate table row for each Global intrinsic rather than using a single row to collectively describe sets of intrinsics such as TypedArray or NativeError . Also intrinsic are that are property of globally named objects are typically named using an underscore after the global component of the name. That suggests for the SIMD constructrors we should have rows (lexical-graphically ordered) for: %SIMD_Bool16x8%, %SIMD_Bool32x4%, %SIMD_Bool8x16%, %SIMD_Float32x4%, %SIMD_Int16x8%, %SIMD_Int32x4%, %SIMD_Int8x16%, %SIMD_UInt32x4%, %SIMD_Int16x8%, %SIMD_UInt8x16%.

Done

This proposal also suggests defined named intrinsics for the built-in functions Array.prototype.joint, Math.abs, Math.fround, Math.max, Math.min, Math.sqrt, Math.imul. So that they maybe invoked by algorithms defined in this proposal. ECMA-262 and ECMA-402 have generally tried to avoid such direct invocations of specific intrinsic functions because because they can be problematic in some situations (for example, they can have realm locality) issues. For each of these I will suggest (in later sections of this review) alternatives that don't require the introduction of additional named intrinsics.

Done (mostly by copying the definitions into new internal algorithms).

##Section 3 (clause 7)

Section 3.1.2 (Subclause 7.1.2) ToBoolean

I'd leave out the NOTE as no comparable explanation is provided for the other argument types. It's find to have in the proposal labeled as "Rationale", but such rationales are not placed in the actual spec. document.

Done

Section 3.1.4 (Subclause 7.1.12) ToString

As a general principal, the definition of primitive operations upon primitive values should only invoke other primitive operations. . This definition of ToString for SIMD values violates that principal by invoking a built-in method (which is not a primitive operations). [Rationale: such a invocation is an up-level dependency within the layered architecture of the specification. This is a potential hazard to future spec. revisions. As changing the definition of a method in the ES library library layer shouldn't implicitly cause a change in the semantics of a primitive value and its associated primitive operations.]

Instead, similarly to the handling of Number values, an algorithm for an abstraction operation (based upon primitive operation) that converts primtive SIMD values to primitive string values should be defined in anew subclause of 7.1.12. such an algorithm is not hard to write. For example, steps 4-9 of 19.2.1.1.1 is an algorithm that converts a List of ECMAScript values to a comma separated string. This can be used as a model for a SIMD ToString algorithm.

Well, I just use the whole definition of Array.prototype.toString. I
don't see much reason to inline a version of that and apply only
slight simplifications, especially since I need to call it twice.

The following comments related to the algorithm currently given in section 3.1.4 and may or may not be applicable to the new algorithm I suggested above:

Step 1: "...an Array containing..." is not a precise enough description if you were actually creating an Array object. Instead if you needed to create an Array you should use CreateArrayFromList where the list is the [[SIMDElements]] field of the SIMD value.

Done

Step 1: ...except that the elements of a [[SIMDElements]] list are not ECMAScript language values (eg, Number or Boolean) and hence can't be directly stored as the values of ECMAScript Array elements. Some where you need to define how each of the possible SIMD element values (eg,Float32, Int16, Bool8, etc. ) convert to/from ECMAScript language values.

I'm not sure where you get that idea. They're supposed to be Number or
Boolean primitives. Did I imply some other way somehow? I specified it
this way since I didn't want to complicate the spec with more notions
of what a number is when it is not needed for the semantics.

Section 3.1.5 (Subclause 7.1.13) ToObject This should probably use a new abstract operation that is more explicit about how a SIMD wrapper object is created for a SIMD primitive value.

That would be reasonable, but I wrote it the way it is currently
written to be parallel to the other clauses of ToObject. I agree that
it'd make sense to rewrite it in terms of ObjectCreate, but seems like
that should be a change against all of them, not just this one.

##Section 3.3 (Subclause 7.2.9) SameValue
If a single SIMD primitive type is used, as suggested above, then step 10 can be rewritten as:

1. If Type(x) is SIMD, then
   a. If x.[[SIMDSubtype]] is different from y.[[SIMDSubtype]], return false.
   b. For i from 0 to x.[[SIMDDescriptor]].[[VectorLength]] - 1, do
   i. If SameValue(SIMDExtractLane(x,i), SIMDExtractLane(y,i)) is false, return false.
   ii.Return true.

Yeah, good point. This is a good argument for the purely spec-internal
SIMD supertype. I'll think about it more.

##Section 3.4 (Subclause 7.2.10) SameValueZero
Do you really want to use SameValueZero to compare the extra vector elements. The only place it would make a difference (in the current ES6 spec) is when using a SIMD value is a Map key or Set element. Do you really want:

let s = new Set;
s.add(Float32x4(0,0,0,0);
console.log(s.has(Float32x4(0, -0, 0, -0))); //do you want this to display true??

It isn't clear that the rationale for why +0 and -0 should be equivalent when used as Map/set keys also applies to the elements of SIMD vectors. Does SIMD hardward generate -0 values for vector elements?

Sure, there are all sorts of ways to get -0 in a SIMD vector, just
like in a scalar. I don't know why Map and Set went with the semantics
that they did, but with that decision made, shouldn't it also apply to
SIMD? Or do you think we should use SameValue for SIMD when in Maps
and Sets?

##Section 3.5 (Subclause 7.2.11) Abstract Relational Comparison

Symbols also throw a TypeError when ToNumber or ToString gets applied to them. I'd either eliminate the NOTE or generalize it so it isn't only about SIMD values.

Eliminated the note.

##Section 3.7 (Subclause 7.2.13) Strict Equality Comparison

So, what is the value equality semantics you intend for SIMD vectors that have NaN element values. Do you really intend that Float32x4(NaN,0,0,0)===Float32x4(NaN,0,0,0) is false

and Float32x4(0,0,0,0)===Float32x4(-0,0,0,0) is true?

Why not? This is a natural generalization of scalar ===. Implementors
have said that they prefer these semantics to SameValue semantics.

Section 6 SIMD

I think API design considerations mentioned at the top of this review would have a great impact upon this sections. Below, I'll comment of the section, as written.

I suggest that the top level of this section be patterned more exactly after 20.3 The Math Object. In other words just describe the %SIMD% intrinsic object. don't talk about the the vrious SIMD properties (ie, constructor here). Also the material about SIMD primitive values should be in clause 6.

Moved the material about SIMD primitive values.

Re: Note 1. Does this actually occur in this spec? I couldn't find and example. It would be cleaner if such lists were never modified... I guess it happens in SIMDcreate. Assuming that is the only place, that's where the note should be.

Lists are modified all over the place. Generally they're created by
making an empty list and appending to it. I felt like the note should
be with the definition of the SIMD so that it's clear that SIMD values
are immutable.

Note 2: The use of fields suggest that they need to be present as part of the runtime value. A table is better because it does not carry that implication.

I am not sure why you would think it carries this implication. Things
are worded as if they're all pointing to the same record. In reality,
the TypedArray spec requires a lot more runtime treatement of these
values than the SIMD spec does. Nothing about records says anything
about them being present at runtime. All a table does is add an extra
indirection through a string name to get to the row.

Last paragraph

This whole paragraph belongs somewhere else. The term "wrapper" isn't used in the ES6 spec when describing such objects. Instead, they are called "NJuber objects", "String objects" etc. You should follow that same pattern. "[[Prototype]] is a SIMDConstructor" can't be correct. It must be an appropriate prototype object. ToObject is not the only way that SIMD objects are created. Another way is using the new operator on one of the SIMD constructors.

Actually, ToObject is the only way that SIMD objects are created (just
like Symbols). We decided this in the last TC39 meeting. I removed all
discussion of 'wrappers'.

Section 6.1 Internal algorithm

You might consider moving these into the definition of SIMD values in clause 6, but they are probably ok as a subclause here.

What part do you think should be moved?

Section 6.1.1 SIMDCreate

"fields' is not a very descriptive parameter name. Consider something like "vectorElements" instead.

Done

You need an assertion about the "descriptor" parameter.

Added one. I can see how it's useful, but to what extent do I need
assertions? Should I put an assertion on all arguments about
everything?

The assertion about "fields" is probably not adequate. You don't actually say it is a List. Besides the length, what else is required of its elements. Are they all primitive values of the type and range supported by the SIMD primitive value implied by the "descriptor"?

Made more of an assertion. The [[Cast]] algorithm takes any ES value,
so I don't think there should be any assertion about what the list
contains.

You have a value domain issue with [[CastNumber]]. In most cases its value is an ECMAScript abstract operation (for example, ToInt32, ToBoolean) but for Float32x4 is is %Math_fround% which is a built-in function. Functions and abstract operations are not the same kind of entities and are can not be invoked interchangeably. The easiest fix is probably to define ToFloat32 as an abstract operation.

Done (though it's called MathFround for now; we can change that).

Also [[CastNumber]] is a misnomer in that you permit ECMAScript Boolean values to be vector elements and Booleans are not Numbers in ES.

Good point, renamed to [[Cast]]

If you MaybeFlushDemormal when creating the vector elements you don't have to do it when you retrieve them. that will simplify other algorithms.

Actually that's on purpose. If you load and then store, for example,
denormals aren't flushed.

Section 6.1.2 SIMDToLane

the NOTE (and similar notes elsewhere) is unnecessary. Implementations can always to such things.

Removed

there should be some sort of assertion about the the type and range of max

What kind of assertion?

Don't use ToInt32 to convert lane values to positive integers. In ES6 we moved away from this because:

ToInt32 does modulo arithmetic on the value, hence large integer values will wrap
It converts Infinity to 0.

Instead we generally use ToLength when we want to cast to a non-negative precise integer value. I'd replace step 3 with:

• Let index be ToLength(lane).
• ReturnIfAbrupt(index).
• If SameValueZero(index, ToNumber(lane)) is false or index ≥ max, throw a RangeError exception.
• Return index.

Oh, infinity is a case I didn't think about. Changed to ToLength.

Section 6.1.4 SIMDReplaceLane

this abstract operation appears to be only called from one place so you might consider inlining it at it's usage site.

Is it a problem like this?

Section 6.1.5 MaybeFlushDemormal

You might consider a type guard in case n is a Boolean value.

Added that

the term "subnormal floating point value number" is not used else where in the spec. Try to minic the language used in 20.1.2.9.

Done

Because n can be an abrupt completion value, I would start both alternatives with the step:

ReturnIfAbrupt(n). #### Section 6.1.6 SIMDBinaryOp

Fixed callers to not invoke it with an abrupt completion.

You have a domain error for op similar to what was discussed for [[CastNumber]] in SIMDCreate. This one is actually worse as there are three different types of incompatible things passed as arguments for this parameter: built-in function objects, abstract operations, mathematical operations (as defined in 5.2. Plus some arguments use higher order abstract operations (SubSaturate, etc) and looks like they have has errors. It's probably best to always pass an abstract operation and if necessary define additional abstract operations to wrap the application of mathematical operations and built-in function invocations.

Fixed domain error; it's all abstract algorithms now.

In step 1 say: "a.[[SIMDTypeDescriptor]] and b.[[SIMDTypeDescriptor]] are the same descriptor"

Done

As noted under SIMDCreate, you shouldn't have to apply MaybeFlushDenormal to the a and b element values because they should have already been added using MaybeFlushDenormal.

No, this is purposeful. Some SIMD values don't have flushed denormals.

Section 6.1.7 SIMDUnaryOp

domain problems with op similar to those noted for SIMDBinaryOp

Fixed.

I don't understand the need to the flushDenormal parameter. It is only false when op is unary minus or the abs function. Does some SIMD hardware treat demoralized values differently for those operations?. It seems like that parameter could be eliminated but maybe I'm missing something.

Yes, hardware treats some ops differently. That's what the spec reflects.

Section 6.1.8 SIMDScalarOp

make sure op is always an abstract operation

Done

Should need to MaybeFlushDenormal for elements of a.

It's always called on integer vectors, so not needed. Added an
assertion for this.

Section 6.1.9/10 SIMDLoad/SIMDStore (also 6.2.51 NOTE)

I strongly recommend passing Data Blocks around in this manner. This was never intended in the original spec. design and it requires significant duplication of spec. material from the Integer Indexed Exotic Objects and Abstract Operations For ArrayBuffers portion of the specification. Such duplication is highly error prone and complicates future spec. maintenance. For example, I see that you are not adequately checking for detached of Array Buffers.
It appears to me, that all places where you do this could be recast to do indexed access to Typed Arrays (possibly directly using IntegerIndexElementGet and IntegerIndexedElementSet. If those prove to be inadequate use SetvalueInBuffer and GetValueFromBuffer

Because of bitcasts, it'd be a bit awkward to always go through
TypedArrays. I refactored it a bit to reduce duplication and fixed the
missing detached check.

###Section 6.2 SIMDConstructor
For consistency (with the Math object organization) this section should probably be titled "Constructor Properties of the SIMD object" and have a subsection for each of the actual constructors.

Done

A constructor(s) and the static methods of that constructor should not be listed at the same list level. See for example 6.2..1 and 6.2.2.

TODO

The same subclause level should not contain both static methods of constructors and abstract operations used by those methods. Either list all of the abstract operations in a separate subclause or place the abstract operation in a subclause of the the method that it is most closely associated with (See 21.1.3.14 for an example of this style.

Moved abstract operations up

Bool SIMD types issues with many of the static methods

The Bool SIMD types are specified such that there vector elements are ECMAScript Boolean values because their [[CastNumber]], which is used when populating the vector, is ToBoolean.

Many of the SIMD static methods (for example add, division) are not restricted from use upon Bool vector values. This means that the provided algorithms try to apply mathematical operator such as + to ECMAScript Boolean values. Such values out not in the domain sets of those operations.

Do you really want to perform arithmetic on Boolean values? If so, you need to specify the semantics of all of the operators that are applied to them.

Oops, no those ops shouldn't exist. Fixed the spec.

Sections 6.2.5, 6.2.5, etc.

"This definition uses the refers to + as defined by..." should probably be ""This definition uses + the refers to the abstact operation defined by...", etc. #### Sections 6.2.6 mul
You can't invoke the built-in function %Math_imul% as if it was an abstract operation

Fixed layering

Sections 6.2.6 div

Aren't there any special semantics for integer divisions??

Removed integer division

Sections 6.2.11, 6.2.12, maxNum/minNum

What's the use case for doring max/min that ignores NaNs? Is it common enough to justify including it in this API
Is it intentional that if both n,m are NaNs that the result is NaN?

Yes, there are use cases, and this is the semantics. @sunfishcode can
you say more?

Sections 6.2.20, abs

Why can't this be applied to integer vectors?

It is just accelerated for floats

Sections 6.2.21, and, etc.

Why can't just reference 12.11 to explain bitwise logical operations because the primary bitwise algorithm in 12.11.3 is a syntactic evaluation routine. The a and b are not parse trees, so you can't apply 12.11.3 to them.
Does SIMD hardware actually support bit-wise operations on Float32 vectors and use the same semantic as ES??

Fixed the layering and removed the float32 bitwise ops.

MORE TO COME

I'm not done commenting on all of the static methods, but I'm pointing this now to give it visibility before the upcoming TC39 meeting. I'll add additional comments in a followup to this issue.

—
Reply to this email directly or view it on GitHub.

[littledan]

OK, I've now renamed MathFround to ToFloat32, and separated out the properties from the constructor. I'm looking forward to the rest of your review.

[littledan]

The more I think about it, the more I like the idea of the table-based approach and a single SIMD type. That would eliminate the confusion with the _SIMD_Descriptor/_SIMD_Type/_SIMD_Constructor alternation. All that's really left is a cleaner way to refer to _SIMD_Constructor.