Closed Bug 1110164 Opened 5 years ago Closed 5 years ago

SIMD.js: Rename select into bitselect, implement select

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(4 files, 1 obsolete file)

So, bug 1031203 made me realize that what's currently called "select" in our impl is now called "bitselect" in the polyfill, and that we don't have in the interpreter what's now called "select" in the polyfill.

Dan, do you think we need the new polyfill "select" in asm.js, for Emscripten? It seems to me that bitselect would be enough, because select can be emulated with bitselect, right?

If that's the case, I'll make this a mentored bug.
Flags: needinfo?(sunfish)
It's true that select can be emulated with bitselect, however it would be nice to support select directly, since element-wise select is what Emscripten will be using most of the time.
Flags: needinfo?(sunfish)
Depends on: 1100123
Attachment #8537203 - Flags: review?(till)
Attachment #8537203 - Flags: review?(sunfish)
Attachment #8537203 - Flags: review?(luke)
/r/1493 - Bug 1110164: Rename MSimdTernaryBitwise into MSimdSelect;
/r/1495 - Bug 1110164: Extend MSimdSelect to also handle element-wise select;
/r/1497 - Bug 1110164: Rename Select into BitSelect in the interpreter and implement Select in the interpreter;
/r/1499 - Bug 1110164: Add bitselect to asm.js;

Pull down these commits:

hg pull review -r 4ac0757d891f48011f68b64d13d3aac0f3dfc211
(to log in into ReviewBoard, you can just use your bugzilla login/password. If you prefer a standard bugzilla review, please let me know and i'll bzexport the patches instead)
/r/1493 - Bug 1110164: Rename MSimdTernaryBitwise into MSimdSelect;
/r/1495 - Bug 1110164: Extend MSimdSelect to also handle element-wise select;
/r/1497 - Bug 1110164: Rename Select into BitSelect in the interpreter and implement Select in the interpreter;
/r/1499 - Bug 1110164: Add bitselect to asm.js;

Pull down these commits:

hg pull review -r ff4effa903e2eca726c22417451ab42ba39765fe
https://reviewboard.mozilla.org/r/1491/#review911

::: js/src/jit/MIR.h
(Diff revision 2)
>  };

It'd be nice to give MSimdSelect a congruenTo method, checking congruentIfOperandsEqual and isElementWise().

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
(Diff revision 2)
> +        if (!mir->mask()->isSimdBinaryComp())

Nice!

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
(Diff revision 2)
> +            masm.vblendvps(mask, onTrue, onFalse, onTrue);

Please add a comment mentioning that SSE4.1 has plain blendvps which can do this, but that it's awkward to use because it requires the mask to be in xmm0.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
(Diff revision 2)
> +            masm.psrad(Imm32(31), mask);

It doesn't appear that Lowering informed the register allocator that mask is being modified here.
https://reviewboard.mozilla.org/r/1491/#review939

> It doesn't appear that Lowering informed the register allocator that mask is being modified here.

Good catch! I will address once the r+'d patch in bug 1100123 gets landed, it will make things easier to deal with here.
Attachment #8537203 - Flags: review?(till) → review+
https://reviewboard.mozilla.org/r/1491/#review1029

I'm slightly bummed-out by the fact that I couldn't find anything worth opening an issue for, and thus can't try out that feature in reviewboard :)

::: js/src/jit-test/tests/asm.js/testSIMD.js
(Diff revision 2)
> +for (var mask of masks)

nit: braces around multi-line block content.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
(Diff revision 2)
> +        // Propagate sign to all bits of mask vector, if necessary

nit: "." at the end of full-sentence comments

::: js/src/tests/ecma_7/SIMD/select-bitselect.js
(Diff revision 2)
> + * https://creativecommons.org/publicdomain/zero/1.0/

CC0 is now the default, so you don't technically need to add this comment

::: js/src/tests/ecma_7/SIMD/select-bitselect.js
(Diff revision 2)
> +    return Array.prototype.map.call(res, (x) => x);

uber-nit: no need for () around arrow-function params, iff at least one param is given

::: js/src/tests/ecma_7/SIMD/select-bitselect.js
(Diff revision 2)
> +function FindCorrespondingScalarTypedArray(type) {

nit: s/Find/find/ to align with the prevailing naming convention.
Attachment #8537203 - Flags: review+ → review?(till)
/r/1493 - Bug 1110164: Rename MSimdTernaryBitwise into MSimdSelect;
/r/1495 - Bug 1110164: Extend MSimdSelect to also handle element-wise select;
/r/1497 - Bug 1110164: Rename Select into BitSelect in the interpreter and implement Select in the interpreter; r=till
/r/1499 - Bug 1110164: Add bitselect to asm.js;

Pull down these commits:

hg pull review -r 6fff1c48db8fb89bc54bf3200239d200642e90c9
Attachment #8537203 - Flags: review?(till)
/r/1493 - Bug 1110164: Rename MSimdTernaryBitwise into MSimdSelect;
/r/1495 - Bug 1110164: Extend MSimdSelect to also handle element-wise select;
/r/1497 - Bug 1110164: Rename Select into BitSelect in the interpreter and implement Select in the interpreter; r=till
/r/1499 - Bug 1110164: Add bitselect to asm.js;

Pull down these commits:

hg pull review -r 6fff1c48db8fb89bc54bf3200239d200642e90c9
Sorry for the mess, experimenting with MozReview, which isn't quite ready in terms of UX... In particular, you don't have to review the entire thing, there are reviews for smaller items:
- https://reviewboard.mozilla.org/r/1493/ and https://reviewboard.mozilla.org/r/1495/ for sunfish
- https://reviewboard.mozilla.org/r/1497/ was for till (thanks for r+!)
- https://reviewboard.mozilla.org/r/1499/ for luke
https://reviewboard.mozilla.org/r/1495/#review1145

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
(Diff revision 2)
> +            // to use because it requires the mask to be in xmm0.

This is a nit, but I'd prefer the comment to be below. The AVX case is fine, it's the non-AVX case where it's theoretically possible to do something better, so we want the comment about why we're not doing it there.
Attachment #8537203 - Flags: review?(sunfish) → review+
Attachment #8537203 - Flags: review?(luke) → review+
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Attachment #8537203 - Attachment is obsolete: true
Attachment #8618896 - Flags: review+
Attachment #8618897 - Flags: review+
Attachment #8618898 - Flags: review+
Attachment #8618899 - Flags: review+
Comment on attachment 8618897 [details]
MozReview Request: Bug 1110164: Add bitselect to asm.js;

https://reviewboard.mozilla.org/r/1499/#review25595
Attachment #8618897 - Flags: review+
You need to log in before you can comment on or make changes to this bug.