Closed Bug 1133859 Opened 5 years ago Closed 5 years ago

SIMD-JS operations are not available on SharedArrayViews.

Categories

(Core :: JavaScript Engine, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jujjyl, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

Attempting to execute SIMD.js operations in Emscripten in conjunction with Lars Hansen's SharedArrayBuffer work, it looks like all SIMD memory operations will fail with an error "TypeError: invalid arguments" when one of the operands is a Shared*Array instead of a regular typed array view.

For the SSE+pthreads demo, this is a bit critical and blocking. Is it easy to lift the restriction and allow Shared*Arrays to be used for SIMD operations?
After discussion with Lars, it appears it is just about accepting shared views as inputs of SIMD.load/store.  In that case, the changes should be pretty small.  Taking.
Assignee: nobody → benj
Status: NEW → ASSIGNED
For AsmJS, there's nothing to change: if you use the SharedUint8Array view, things work fine.  Note that is the only array view size for which it will work, as SIMD.load/store (in asm.js) expects an uint8arrayview (so as not to have to implicitly do the pointer shift).


function f(glob, ffi, heap) {
    "use asm";
    var su8 = new glob.SharedUint8Array(heap);
    var f4 = glob.SIMD.float32x4;
    var load = f4.load;
    function g() {
        return load(su8, 0);
    }
    return g;
}
Nothing needs to be changed in asm.js as stated in the previous comment.  This
implements support in the interpreter and adds basic test cases (the code paths
are entirely shared with non-shared views, so adding tests wouldn't have a lot
of worth, unless i am missing something).
Attachment #8565905 - Flags: review?(lhansen)
Comment on attachment 8565905 [details] [diff] [review]
Allow shared array views as SIMD.load/store arguments

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

Nit: In load.js I would initialize the storage to something predictable (I know, tricky the way the test is written) and test that the values read are the values expected, not zero - since zero is an easy default for a broken implementation to supply.
Attachment #8565905 - Flags: review?(lhansen) → review+
Thanks, I could adapt the framework contained in this test file to handle properly SAB, and could test at a few positions all load kinds.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14b0ae5fbda5
Thanks for the patch Benjamin, tested it out and it works flawlessly!
https://hg.mozilla.org/mozilla-central/rev/161af2aba1e9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(In reply to Florian Scholz [:fscholz] (MDN) from comment #9)
> Shared*Arrays are not yet documented, but for SIMD I added them as inputs to
> the load/store pages:
> 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

I don't have any strong opinions either way, but the reason those arrays are not documented is that there is not full agreement on the design yet (including not just names but whether they exist at all).  So you want to be a little careful about creating references to them.
(In reply to Lars T Hansen [:lth] from comment #10)
> I don't have any strong opinions either way, but the reason those arrays are
> not documented is that there is not full agreement on the design yet
> (including not just names but whether they exist at all).  So you want to be
> a little careful about creating references to them.

Thanks, I am still on hold there :-) I am trying to follow discussions, implementation status in engines etc., but putting the "dev-doc-needed" keyword on bugs with API changes (and providing some notes like what you just wrote) is always a big help!
You need to log in before you can comment on or make changes to this bug.