Closed Bug 1155211 Opened 5 years ago Closed 5 years ago

SIMD: rename lane mutators

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- affected
firefox42 --- fixed

People

(Reporter: bbouvier, Assigned: fiji, Mentored)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [good first bug][lang=c++][DocArea=JS])

Attachments

(8 files, 11 obsolete files)

52.64 KB, patch
fiji
: review+
Details | Diff | Splinter Review
34.60 KB, patch
fiji
: review+
Details | Diff | Splinter Review
65.93 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
6.74 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
22.85 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
14.08 KB, patch
nbp
: review+
Details | Diff | Splinter Review
24.27 KB, patch
nbp
: review+
Details | Diff | Splinter Review
17.66 KB, patch
nbp
: review+
Details | Diff | Splinter Review
In [0], following changes have been proposed and seem to get acceptance:
- rename {store,load}{x,xy,xyz} into {store,load}{1,2,3}
- change the getter methods to extractLane(index) static methods
- change the setter methods (with{X,Y,Z,W}) to an insertLane(index, value) static method

Names might not be final at this point, but bullet 1 can be done in a nicely small separated patch. I'm willing to help mentoring this bug, if somebody is interested in trying to fix it. First, you'll need to modify the C++ code and then modify all tests (JIT tests and jstests), and make sure they all pass in a debug build of spidermonkey.

[0] https://github.com/johnmccutchan/ecmascript_simd/issues/110
I want to work on this bug.
(In reply to divya23.gaur from comment #1)
> I want to work on this bug.

Please go ahead :) This bug's assignee will be the first person who submits a patch. If you need any help, don't hesitate to ask here or in #jsapi on irc.mozilla.org.
Hi , I want to work on this bug.
I would like to work on this. I've attached a patch for the first bullet (rename {store,load}{x,xy,xyz} into {store,load}{1,2,3}).

I'll keep working on the other two changes.
Attachment #8597584 - Flags: feedback?(benj)
Attachment #8597584 - Attachment is obsolete: true
Attachment #8597584 - Flags: feedback?(benj)
Recent updates on this bug, is it still open to work?
Just to get this right. Changing the the getter methods to an extractLane static method would result in js code like:

let a = SIMD.int32x4(1, 2, 3, 4);
let x = SIMD.int32x4.extractLane(a, 1);  // instead of let x = a.x;
let b = SIMD.int32x4.insertLane(a, 1, 2);  // instead of let b = SIMD.int32x4.withX(a, 2);

Is that right?

By the way, As of https://github.com/johnmccutchan/ecmascript_simd/issues/110 the consensus was .replaceLane()/.extractLane(). This should probably be reflected in this bug.
Flags: needinfo?(benj)
(In reply to Florian Merz from comment #7)
> Just to get this right. Changing the the getter methods to an extractLane
> static method would result in js code like:
> 
> let a = SIMD.int32x4(1, 2, 3, 4);
> let x = SIMD.int32x4.extractLane(a, 1);  // instead of let x = a.x;
> let b = SIMD.int32x4.insertLane(a, 1, 2);  // instead of let b =
> SIMD.int32x4.withX(a, 2);
> 
> Is that right?
> 
> By the way, As of
> https://github.com/johnmccutchan/ecmascript_simd/issues/110 the consensus
> was .replaceLane()/.extractLane(). This should probably be reflected in this
> bug.

That's right! Feel free to jump in if you're willing to change this as well (in a separate patch from the first bullet, please, to make reviews simpler). Regarding extractLane/replaceLane: I'd wait for the changes to be reflected on the spec repository before working on the final name, as many tests will be affected by this change.
Flags: needinfo?(benj)
This should take care of the first bullet.

I'll continue on the other two changes as soon as the changes made it into the ecmascript_7 repository.
Attachment #8597630 - Attachment is obsolete: true
Attachment #8598750 - Flags: feedback?(benj)
Comment on attachment 8598750 [details] [diff] [review]
Bug_1155211_SIMD_rename_lane_mutators_store_load.patch

Review of attachment 8598750 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for jumping in! Can you please run the jit-tests as well and update your patch? (from the js/src/ directory, jit-test/jit_tests.py path/to/bin/js)
Attachment #8598750 - Flags: feedback?(benj)
Now the patch includes jit-tests.

Getters and setters are still missing in the ecmascript_simd repo. Therefore, I'm still wating with the other two bullets.
Attachment #8598750 - Attachment is obsolete: true
Attachment #8600558 - Flags: review?(benj)
Comment on attachment 8600558 [details] [diff] [review]
Bug_1155211_SIMD_rename_lane_mutators_store_load.patch

Review of attachment 8600558 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thank you! I'd like to have more alignment, but that's really just a nice to have, so not blocking review on this. However, if you want to address the style issues, you can fix these locally, upload a new variant of the patch and carry forward the r+ (that is, no need to ask for another review for the updated patch).

Please send this to try (and if you don't have try access, follow instructions as marked on [0] and CC me on the try rights bug), so that we make sure that all tests pass.

[0] https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

::: js/src/builtin/SIMD.h
@@ +45,5 @@
>    V(lessThanOrEqual, (CompareFunc<Float32x4, LessThanOrEqual>), 2)                    \
>    V(load,    (Load<Float32x4, 4>), 2)                                                 \
> +  V(load3, (Load<Float32x4, 3>), 2)                                                   \
> +  V(load2,  (Load<Float32x4, 2>), 2)                                                  \
> +  V(load1,   (Load<Float32x4, 1>), 2)                                                 \

Here and below, can you please keep parenthesis alignment?

::: js/src/jit-test/tests/asm.js/testSIMD-load-store.js
@@ +335,5 @@
>      }
>  
>      // Test correct loads
>      var i = 0, j = 0; // i in elems, j in bytes
> +    assertEqX4(m.load1(j),   [x(i), 0, 0, 0]);

uber-nit: Here and below, can you keep alignment at start of arrays, please?
Attachment #8600558 - Flags: review?(benj) → review+
Another simple task that can fit in this bug: [0], which renames bitselect into selectBits.

https://github.com/johnmccutchan/ecmascript_simd/issues/140
Fixed alignment. I'll send it to the try server once I've got the permissions. Thanks for vouching,

I will look at the bitselect renaming while waiting for the getter/setter stuff being worked out.
Attachment #8600558 - Attachment is obsolete: true
Attachment #8601115 - Flags: review+
(In reply to Florian Merz from comment #15)
> Ran on try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=267b67c75c64

Try looks good! Tested platforms are the expected ones. Can you add "; r=bbouvier" at the end of the commit changeset, and then we can put `checkin-needed` in the whiteboard, so that a sheriff pushes that to inbound?
Assignee: nobody → flomerz
Status: NEW → ASSIGNED
Updated the commit message
Attachment #8601115 - Attachment is obsolete: true
Attachment #8601667 - Flags: review+
QA Whiteboard: checkin-needed
QA Whiteboard: checkin-needed
The lane mutators getter and setter patch has been pushed to the ecmascript_simd repo yesterday (https://github.com/johnmccutchan/ecmascript_simd/commit/3ecea43476349f5ca0968144a06f060076e387d6). I will start working on it. That will take some time though.
Once the bitselect -> selectBits patch has landed there, I will tackle that, too.
Just to give you an update. I'm still working on the setter part. It is a little bit tricky since it changes the behavior from a lane being bound to the operation to the lane being a parameter. This makes it a little bit more difficult to adopt the jit part.

I didn't have much time last week, but there's a long weekend coming. Hence, I'll have quite some time this week.
Keywords: dev-doc-needed
Whiteboard: [good first bug][lang=c++] → [good first bug][lang=c++][DocArea=JS]
Here is a preliminary version for the setters (with{X,Y,Z,W} => replaceLane). Feedback would be welcome since it touches a lot of code.


For now the SSE4.1 code is disabled in:
jit/x86-shared/CodeGenerator-x86-shared.cpp

It will be some work for me to adopt the SSE4.1 instructions to the API change. But I'll give it a try.
Attachment #8606617 - Flags: feedback?(benj)
Comment on attachment 8606617 [details] [diff] [review]
Bug_1155211_SIMD_rename_lane_mutators_setters.patch

Review of attachment 8606617 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the long time needed to answer! While your work really looks great, I don't think the case of non-constant extractLane should be optimized, at least at the beginning. Moreover, there doesn't seem to be any x86 instruction that efficiently does the non-constant lane extraction. Here's my proposal: keep the MIR/LIR/Lowering/etc parts around for the future, in case we have use-cases where we need to optimize non constant accesses; but in the meanwhile, you can just modify the code such that non-constant index doesn't get optimized (in IonBuilder, don't optimize if the lane index isn't a MConstant, then map the value of the constant to the corresponding SimdLane enum value; same goes in AsmJSValidate). This way, you can keep most of the code (MIR/LIR/Lowering etc.) as it is right now, and stay in the scope of this original bug, which was supposed to be renaming (sorry this got so much out of scope). How does that sound?
Attachment #8606617 - Flags: feedback?(benj)
Sounds great Benjamin. Thanks for the input I'll get to work and get back to you. And you're right, optimizing only if the lane is a constant makes it way easier (for the getters to). I'll adjust the code for the setters and continue with the getters and selectBits.
I've remove the optimization for the non-constant case. Also I added a unit test asserting that asm.js does not compile if the lane is not constant.

My only concern is that some thing like:

const lane = 0; SIMD.int32x4.replaceLane(someInt32x4Val, lane, 1);

does not complie to asm.js code. Why is this treated different, than:

SIMD.int32x4.replaceLane(someInt32x4Val, 0, 1);


I'll continue with the getters.
Attachment #8606617 - Attachment is obsolete: true
Attachment #8613714 - Flags: review?(benj)
The SIMD.js spec is going through some pretty major changes right now. asm.js and Firefox's implementation are not yet up to date with all the latest changes, but we are planning on resyncing everything.
Comment on attachment 8613714 [details] [diff] [review]
Bug_1155211_SIMD_rename_lane_mutators_setters.patch

Review of attachment 8613714 [details] [diff] [review]:
-----------------------------------------------------------------

I realize this is harder than what should fit in a "good first bug", sorry about that. Also, you're doing great :) I would like to give another look, once a few checks and tests have been added.

::: js/src/asmjs/AsmJSValidate.cpp
@@ +5617,5 @@
>          return CheckSimdScalarArgs(formalSimdType_)(f, arg, argIndex, actualType, def);
>      }
>  };
>  
> +class CheckSimdVectorLaneScalarArgs

It's very specific to replaceLane, so I'd just call it CheckSimdReplaceLaneArgs

@@ +5630,5 @@
> +    {
> +        MOZ_ASSERT(argIndex < 3);
> +        uint32_t u32;
> +        switch (argIndex) {
> +            case 0:

nit: our style guide requires that the "case" keyword are 2 spaces after the column start of the switch, and the case body is 2 spaces after the "case" keyword:

switch (something) {
  case 0:
    doSomething();
    break;
  case 1:
...

@@ +5641,5 @@
> +            case 1:
> +                // Second argument is the lane < 4
> +                if (!IsLiteralOrConstInt(f, arg, &u32))
> +                    return f.failf(arg, "lane selector should be a constant integer literal");
> +                return u32 < 4;

This will fail silently if u32 >= 4; Instead can you do

if (u32 >= SimdTypeToLength(formalSimdType_))
    return f.failf(arg, "lane selector should be in bounds");

@@ +5718,2 @@
>      *type = opType;
> +    *def = f.insertElementSimd(defs[0], defs[2], (SimdLane)lane, type->toMIRType());

nit: C++ conversion here, please: SimdLane(lane)

::: js/src/builtin/SIMD.h
@@ +58,5 @@
>    V(store3, (Store<Float32x4, 3>), 3)                                                 \
>    V(store2, (Store<Float32x4, 2>), 3)                                                 \
>    V(store1, (Store<Float32x4, 1>), 3)                                                 \
>    V(sub, (BinaryFunc<Float32x4, Sub, Float32x4>), 2)                                  \
> +  V(replaceLane, (ReplaceLane<Float32x4>), 3)                                         \

nit: please keep alphabetical order here

@@ +108,5 @@
>    V(notEqual, (CompareFunc<Float64x2, NotEqual>), 2)                                  \
>    V(store,  (Store<Float64x2, 2>), 3)                                                 \
>    V(store1, (Store<Float64x2, 1>), 3)                                                 \
>    V(sub, (BinaryFunc<Float64x2, Sub, Float64x2>), 2)                                  \
> +  V(replaceLane, (ReplaceLane<Float64x2>), 3)                                         \

ditto

@@ +158,5 @@
>    V(store,  (Store<Int32x4, 4>), 3)                                                   \
>    V(store3, (Store<Int32x4, 3>), 3)                                                   \
>    V(store2, (Store<Int32x4, 2>), 3)                                                   \
>    V(store1, (Store<Int32x4, 1>), 3)                                                   \
> +  V(replaceLane, (ReplaceLane<Int32x4>), 3)                                           \

ditto

::: js/src/jit-test/tests/SIMD/replacelane.js
@@ +19,1 @@
>      }

please add tests that fail, as in the other test file.

::: js/src/jit-test/tests/asm.js/testSIMD.js
@@ +615,5 @@
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + "function f() {var x = f4(1,2,3,4); x = r(x, 0, x);} return f");
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); x = r(1, 0, f32(1));} return f");
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); x = r(1, 0., f32(1));} return f");
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); x = r(f32(1), 0, f32(1));} return f");
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); var l = 0; x = r(x, l, f32(1));} return f");

Not sure to see what the issue is with this one?

@@ +616,5 @@
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); x = r(1, 0, f32(1));} return f");
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); x = r(1, 0., f32(1));} return f");
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); x = r(f32(1), 0, f32(1));} return f");
> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); var l = 0; x = r(x, l, f32(1));} return f");
> +assertAsmTypeFail('glob', USE_ASM + I32 + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); var y = i4(1,2,3,4); x = r(y, 0, f32(1));} return f");

Can you add tests for the cases where the lane is:
- not an int32
- not between 0 and 4
- not a constant value

::: js/src/jit/MCallOptimize.cpp
@@ +313,5 @@
>  
>      COMP_COMMONX4_TO_INT32X4_SIMD_OP(INLINE_SIMD_COMPARISON_)
>  #undef INLINE_SIMD_COMPARISON_
>  
> +    if (native == js::simd_int32x4_replaceLane)                                                    \

nit: here and below, no need for \ at the end of the line

@@ +3376,5 @@
>  }
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineSimdReplaceLane(CallInfo& callInfo, JSNative native,
> +                                  SimdTypeDescr::Type type)

can it fit on one line? (less than 99 chars)

@@ +3384,5 @@
>          return InliningStatus_NotInlined;
>  
> +    MDefinition* arg = callInfo.getArg(1);
> +    if (!arg->isConstantValue())
> +        return InliningStatus_NotInlined;

You need to add a check that the constant value is an int32 + that this int32 is between 0 and 4. Please also add tests that would have caught this.

@@ +3385,5 @@
>  
> +    MDefinition* arg = callInfo.getArg(1);
> +    if (!arg->isConstantValue())
> +        return InliningStatus_NotInlined;
> +    SimdLane lane = (SimdLane)callInfo.getArg(1)->constantValue().toInt32();

nit: C++ conversion: SimdLane lane = SimdLane(blah);

::: js/src/tests/ecma_7/SIMD/with.js
@@ +1,1 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))

feel free to rename the file as replaceLane.js

@@ +3,5 @@
>  var float32x4 = SIMD.float32x4;
>  var int32x4 = SIMD.int32x4;
>  var float64x2 = SIMD.float64x2;
>  
>  var summary = 'with{X,Y,Z,W}';

nit: please update "summary" or delete it and its uses

@@ +23,4 @@
>      return [arr[0], arr[1], arr[2], x];
>  }
>  
>  function testWith(vec, scalar, simdFunc, func) {

nit: please rename this function

@@ +59,5 @@
>  
>    var v = float32x4inputs[1][0];
> +  assertEqX4(float32x4.replaceLane(v, 0), replaceLane0(simdToArray(v), NaN));
> +  assertEqX4(float32x4.replaceLane(v, 0, good), replaceLane0(simdToArray(v), good | 0));
> +  assertThrowsInstanceOf(() => float32x4.replaceLane(v, 0, bad), TestError);

Also need tests that should throw: the lane's index is non-int32 or an out-of-bounds int32.
Attachment #8613714 - Flags: review?(benj) → feedback+
(In reply to Benjamin Bouvier [:bbouvier] from comment #27)

Thanks for the detailed input Benjamin. I'll adopt the patch to your comments and add the tests. I've also made progress on the getters. It was less effort than I thought it would be. But there will be quite a big change regarding the tests.

I'll attach the new patches as soon as I'm done.
Blocks: 1173722
I hope I took care of all your comments.

> +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); var l = 0; x = r(x, l, f32(1));} return f");

I would say this one fails because l is not a literal. And I guess this behaviour is intended. At least thats the way I implemented it.

However, in the I replace "var l = 0;" with "const l = 0;" it still fails. We currently optimize only for literals not for constants.

I'll move on the extractLane.
Attachment #8613714 - Attachment is obsolete: true
Attachment #8622003 - Flags: review?(benj)
(In reply to Florian Merz from comment #29)
> Created attachment 8622003 [details] [diff] [review]
> Bug_1155211_SIMD_rename_lane_mutators_setters.patch
> 
> I hope I took care of all your comments.
> 
> > +assertAsmTypeFail('glob', USE_ASM + F32 + RLF + FROUND + "function f() {var x = f4(1,2,3,4); var l = 0; x = r(x, l, f32(1));} return f");
> 
> I would say this one fails because l is not a literal. And I guess this
> behaviour is intended. At least thats the way I implemented it.
Good catch.
> 
> However, in the I replace "var l = 0;" with "const l = 0;" it still fails.
> We currently optimize only for literals not for constants.
That's expected, because a global constant literal in asm.js is simply considered as a literal that will be copied every time it gets used.
Comment on attachment 8622003 [details] [diff] [review]
Bug_1155211_SIMD_rename_lane_mutators_setters.patch

Review of attachment 8622003 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Please fix these last comments, upload a new patch (no need to ask for another review, you can carry forward the r+), and if tests run fine locally (I'd had thought the asm.js/simd-* test files would need some updates as well), send it to the try server.

::: js/src/builtin/SIMD.h
@@ +53,5 @@
>    V(minNum, (BinaryFunc<Float32x4, MinNum, Float32x4>), 2)                            \
>    V(mul, (BinaryFunc<Float32x4, Mul, Float32x4>), 2)                                  \
>    V(notEqual, (CompareFunc<Float32x4, NotEqual>), 2)                                  \
>    V(or, (CoercedBinaryFunc<Float32x4, Int32x4, Or, Float32x4>), 2)                    \
> +  V(replaceLane, (ReplaceLane<Float32x4>), 3)                                         \

Can you please move it to the ternary function list?

@@ +105,5 @@
>    V(min, (BinaryFunc<Float64x2, Minimum, Float64x2>), 2)                              \
>    V(minNum, (BinaryFunc<Float64x2, MinNum, Float64x2>), 2)                            \
>    V(mul, (BinaryFunc<Float64x2, Mul, Float64x2>), 2)                                  \
>    V(notEqual, (CompareFunc<Float64x2, NotEqual>), 2)                                  \
> +  V(replaceLane, (ReplaceLane<Float64x2>), 3)                                         \

ditto

@@ +150,5 @@
>    V(load1, (Load<Int32x4, 1>), 2)                                                     \
>    V(mul, (BinaryFunc<Int32x4, Mul, Int32x4>), 2)                                      \
>    V(notEqual, (CompareFunc<Int32x4, NotEqual>), 2)                                    \
>    V(or, (BinaryFunc<Int32x4, Or, Int32x4>), 2)                                        \
> +  V(replaceLane, (ReplaceLane<Int32x4>), 3)                                           \

ditto

::: js/src/jit/MCallOptimize.cpp
@@ +3201,5 @@
>  
> +    MDefinition* arg = callInfo.getArg(1);
> +    if (!arg->isConstantValue() || arg->type() != MIRType_Int32)
> +        return InliningStatus_NotInlined;
> +    int32_t lane = callInfo.getArg(1)->constantValue().toInt32();

You can reuse arg here, instead of calling callInfo.getArg(1) again
Attachment #8622003 - Flags: review?(benj) → review+
As requested, my current state of extractLane.
Attachment #8622924 - Flags: feedback?(benj)
I adopted the patch to your latest comments and push it to the try server.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=457118d76b85
Attachment #8622003 - Attachment is obsolete: true
Attachment #8623285 - Flags: review+
All green, let's get it landed!

Sheriffs, can you please land Bug_1155211_SIMD_rename_lane_mutators_setters.patch ? Try run in comment 33. Thanks!
Keywords: checkin-needed
Comment on attachment 8622924 [details] [diff] [review]
Bug_1155211_SIMD_rename_lane_mutators_getters.patch

Review of attachment 8622924 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this patch! As discussed by email, I'll take over the rest of the patch (many tests to change and there's still some time pressure on my side). Of course, I'll keep your name as the patch author.

These review comments are mainly reminders to myself.

TODO:
- remove the lane getters in the interpreter
- remove the try getprop simd code in baseline or ion
- add tests for non valid lanes, in the interpreter and in the jits

::: js/src/asmjs/AsmJSValidate.cpp
@@ +5630,5 @@
> +                    MDefinition** def) const
> +    {
> +        MOZ_ASSERT(argIndex < 2);
> +        uint32_t u32;
> +        switch (argIndex) {

nit: use an if here (only two cases)

@@ +5639,5 @@
> +                               Type(formalSimdType_).toChars());
> +            }
> +            return true;
> +          case 1:
> +            // Second argument is the lane < 4

nit: 4 => vector length

@@ +5646,5 @@
> +            if (u32 >= SimdTypeToLength(formalSimdType_))
> +                return f.failf(arg, "lane selector should be in bounds");
> +            return true;
> +        }
> +        return false;

failing with no error message here

@@ +5748,5 @@
> +    if (!CheckSimdCallArgs(f, call, 2, CheckSimdExtractLaneArgs(opType), &defs))
> +        return false;
> +    switch (opType) {
> +      case AsmJSSimdType_int32x4:   *type = Type::Signed; break;
> +      case AsmJSSimdType_float32x4: *type = Type::Float;  break;

There has to be a helper for this, or this should be one :)

::: js/src/builtin/SIMD.cpp
@@ +702,5 @@
>  }
>  
>  template<typename V>
>  static bool
> +ExtractLane(JSContext* cx, unsigned argc, Value* vp)

also, we need to remove all the lane getters in this file (and corresponding code in the JITs)

@@ +711,5 @@
> +    // Only the first and second arguments are mandatory
> +    if (args.length() < 2 || !IsVectorObject<V>(args[0]))
> +        return ErrorBadArgs(cx);
> +
> +    Elem* vec = TypedObjectMemory<Elem*>(args[0]);

nit: can be done further down

::: js/src/jit/MCallOptimize.cpp
@@ +3198,5 @@
>  }
>  
>  IonBuilder::InliningStatus
> +IonBuilder::inlineSimdExtractLane(CallInfo& callInfo, JSNative native,
> +                                  SimdTypeDescr::Type type)

nit: can surely fit on one line

@@ +3202,5 @@
> +                                  SimdTypeDescr::Type type)
> +{
> +    InlineTypedObject* templateObj = nullptr;
> +    if (!checkInlineSimd(callInfo, native, type, 2, &templateObj))
> +        return InliningStatus_NotInlined;

nit: maybe give a reason for not inlining
Attachment #8622924 - Flags: feedback?(benj) → feedback+
Attached patch 3.1 Implement extractLane (obsolete) — Splinter Review
Before, if you wanted to extract a lane value from a SIMD vector, you'd do like this:

var v = SIMD.Int32x4(1,2,3,4);
var laneValue = v.x; // 1
var another = v.z; // 3

With the latest spec, one has to do like this:

var v = SIMD.Int32x4(1,2,3,4);
var laneValue = SIMD.Int32x4.extractLane(v, 0); // 1
var another = SIMD.Int32x4.extractLane(v, 2); // 3

This patch implements this, in the interpreter, Ion and Odin. Note that the JITs are restricted to inline only if the lane index is a constant int32.

Detailed spec in the polyfill:
https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd.js#L1683-L1699
Attachment #8622924 - Attachment is obsolete: true
Attachment #8629411 - Flags: review?(hv1989)
Attached patch 3.2 TestsSplinter Review
This expects a rubberstamp rather than a full review, as it changes the tests to the new syntax ;)
Attachment #8629412 - Flags: review?(hv1989)
Attached patch Part 3.3: Remove the getters (obsolete) — Splinter Review
This removes the SIMD getters in the interpreter, in Odin, and their inlining in Ion.
Attachment #8629413 - Flags: review?(nicolas.b.pierron)
Attached patch 3.4: New testsSplinter Review
While patch 3.2 didn't change anything in the behavior of the tests, nor didn't it try to add new tests, this one patch does add new tests, for testing the corner case values.
Attachment #8629414 - Flags: review?(hv1989)
Some parts of this patch had to be in the part 3.3. See previous comment for the explanation and spec of this function.
Attachment #8629411 - Attachment is obsolete: true
Attachment #8629411 - Flags: review?(hv1989)
Attachment #8629417 - Flags: review?(hv1989)
Attachment #8629413 - Attachment is obsolete: true
Attachment #8629413 - Flags: review?(nicolas.b.pierron)
Attachment #8629418 - Flags: review?(hv1989)
Comment on attachment 8629418 [details] [diff] [review]
3.3. Remove the getters

As nbp actually reviewed all that code which gets deleted (and he was selected as the first reviewer, as he noticed on irc --thanks!), he should be the one reviewing this one.
Attachment #8629418 - Flags: review?(hv1989) → review?(nicolas.b.pierron)
Just a renaming patch, which will end the patch series of this bug: this renames bitselect into selectBits, as done in the spec (example for int32x4: https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd.js#L3693-L3711 )
Attachment #8629860 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8629418 [details] [diff] [review]
3.3. Remove the getters

Review of attachment 8629418 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonBuilder.cpp
@@ -10365,5 @@
> -    const JSAtomState& names = compartment->runtime()->names();
> -
> -    // Reading the signMask property.
> -    if (name == names.signMask) {
> -        MSimdSignMask* ins = MSimdSignMask::New(alloc(), obj, type);

Shouldn't we keep this function to optimize .signMask, in a similar way as in CheckDotAccess?
Attachment #8629418 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #46)
> Comment on attachment 8629418 [details] [diff] [review]
> 3.3. Remove the getters
> 
> Review of attachment 8629418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/IonBuilder.cpp
> @@ -10365,5 @@
> > -    const JSAtomState& names = compartment->runtime()->names();
> > -
> > -    // Reading the signMask property.
> > -    if (name == names.signMask) {
> > -        MSimdSignMask* ins = MSimdSignMask::New(alloc(), obj, type);
> 
> Shouldn't we keep this function to optimize .signMask, in a similar way as
> in CheckDotAccess?

Thanks for the review!
Good call; signMask is expected to be deleted too (when bool vectors are implemented in the other bug). If I land the patches of this bug without the bool vectors, I'll keep the function.
Comment on attachment 8629860 [details] [diff] [review]
4. Rename bitselect into selectBits

Review of attachment 8629860 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_7/SIMD/select-bitselect.js
@@ +73,5 @@
>      var ScalarTypedArray = findCorrespondingScalarTypedArray(type);
>      for (var i = 0; i < 16; i++) {
>          var mask = Int32x4.bool(!!(i & 1), !!((i >> 1) & 1), !!((i >> 2) & 1), !!((i >> 3) & 1));
>          for ([x, y] of inputs)
> +            assertEqVec(type.selectBits(mask, x, y), selectBits(type, mask, x, y));

nit: This assertion does not match the comment.  Can you add an assertion such as:

   assertEqVec(type.selectBits(mask, x, y), type.select(mask, x, y));
Attachment #8629860 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #48)
> Comment on attachment 8629860 [details] [diff] [review]
> 4. Rename bitselect into selectBits
> 
> Review of attachment 8629860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/tests/ecma_7/SIMD/select-bitselect.js
> @@ +73,5 @@
> >      var ScalarTypedArray = findCorrespondingScalarTypedArray(type);
> >      for (var i = 0; i < 16; i++) {
> >          var mask = Int32x4.bool(!!(i & 1), !!((i >> 1) & 1), !!((i >> 2) & 1), !!((i >> 3) & 1));
> >          for ([x, y] of inputs)
> > +            assertEqVec(type.selectBits(mask, x, y), selectBits(type, mask, x, y));
> 
> nit: This assertion does not match the comment.  Can you add an assertion
> such as:
> 
>    assertEqVec(type.selectBits(mask, x, y), type.select(mask, x, y));

In the testing mindset, this would make a test conformance rely on another thing being tested in the same file / context. Sure that these two assertions can add up, though. Thanks for the review.
Thank you for your last review comment! I actually changed the assertion, and it wasn't working for float64x2 (quite obviously, when you come to think about it). So I checked the spec, and actually selectBits is now implemented only on int32x4 (and other integer types we don't have in the interpreter so far). So we should remove the float implementations of selectBits. This follow-up patch does that.
Attachment #8629891 - Flags: review?(nicolas.b.pierron)
Attachment #8629891 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8629417 [details] [diff] [review]
3.1 Implement extractLane

Review of attachment 8629417 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/AsmJSValidate.cpp
@@ +5779,5 @@
> +        return false;
> +
> +    switch (opType) {
> +      case AsmJSSimdType_int32x4:   *type = Type::Signed; break;
> +      case AsmJSSimdType_float32x4: *type = Type::Float;  break;

No float64x2 here?

::: js/src/builtin/SIMD.cpp
@@ +709,5 @@
> +
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    // Only the first and second arguments are mandatory
> +    if (args.length() < 2 || !IsVectorObject<V>(args[0]) || !args[1].isInt32())
> +        return ErrorBadArgs(cx);

For now, shouldn't we test that if the type is double, if it can get converted to int32 without loss and use that?

For JS we only know number. So we can use int32 and double interchangeable. And we don't guarantee anything about what we will internally provide.

E.g:
var a = [foo | 0]; // try to make sure foo is in a integer format
a.map(function(e) { 
   e  // could be and will most likely be double here...
   SIMD.float32x4.extractLane(..., e);  // will fail!
});

We should here and in all other functions test that 
args[1].isInt32() || (args[1].isDouble() && (int32)args[1].toDouble() == args[1].toDouble())

::: js/src/jit/MCallOptimize.cpp
@@ +316,5 @@
>  #undef INLINE_SIMD_COMPARISON_
>  
> +    if (native == js::simd_int32x4_extractLane)
> +        return inlineSimdExtractLane(callInfo, native, SimdTypeDescr::Int32x4);
> +    if (native == js::simd_float32x4_extractLane)

No simd_float64x2_extractLane inline?
Attachment #8629417 - Flags: review?(hv1989) → review+
Attachment #8629412 - Flags: review?(hv1989) → review+
Attachment #8629414 - Flags: review?(hv1989) → review+
Keywords: leave-open
(In reply to Florian Scholz [:fscholz] (MDN) from comment #54)
> Documentation updates:
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/SIMD/load
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/SIMD/store
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/SIMD/extractLane
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/SIMD/replaceLane
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/SIMD/selectBits
> 
> Review appreciated.

Thanks! Looks good.
You need to log in before you can comment on or make changes to this bug.