Closed Bug 1088709 Opened 10 years ago Closed 10 years ago

SIMD: Add {float32x4,int32x4}.{load,store} to the interpreter

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

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

Attachments

(1 file, 1 obsolete file)

This spec [0] seems to have leverage now.

Dan, how should we implement partial out of bounds accesses?

var x = new Int32Array(4);
var i4 = SIMD.int32x4.load(x, 2); // x[4] and x[5] are out of bounds. Should we load 0 as in the interpreter (what the polyfill would do now), or just throw at runtime?

[0] https://github.com/johnmccutchan/ecmascript_simd/issues/78
Flags: needinfo?(sunfish)
The current polyfill throws in this situation, and is also the plan for the new polyfill:

https://github.com/johnmccutchan/ecmascript_simd/pull/79
Flags: needinfo?(sunfish)
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Attached patch WIP patch + tests (obsolete) — Splinter Review
Not setting r? for now, as spec hasn't entirely get stable. If the current impl with ArrayBuffer and byteOffset is kept rather than this impl, we can reuse almost all code. (funny fact: the C++ code for arguments checking almost directly mapped to the JS code, although I didn't look at the JS code beforehand!)

This will need more tests before asking for r? as well.
Attached patch Load / StoreSplinter Review
Till, I've chosen you as reviewer as you did some reviews for SIMD recently, let me know if you don't want to do them anymore and / or feel free to bounce the review to anybody else :)

Spec is at https://github.com/johnmccutchan/ecmascript_simd/issues/78

There is only one thing i've changed compared to the spec: for SIMD.*.store, it makes sense that the returned value is the stored value. This makes it symmetric with any TypedArray store (where f32[i] = v; returns v and allow forms like (var x = f32[i] = v;)). I'll comment in the PR.

As load and store are pretty symmetric wrt to the checks they do on their arguments and their internal behavior, I've made the choice to extensively test load, and have only basic tests for store. I actually think that "load" tests are pretty exhaustive, testing SIMD.{int32x4,float32x4}.load(TA, i) for all kinds of typed arrays TA and interesting indexes i. Hope you'll appreciate the tests :)
Attachment #8512054 - Attachment is obsolete: true
Attachment #8515039 - Flags: review?(till)
Dan: note these tests could be reused in your PR i think, as they test *all* kind of typed arrays (for load at least).
Status: NEW → ASSIGNED
Comment on attachment 8515039 [details] [diff] [review]
Load / Store

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

I think better error messages for invalid arguments would be very nice. We have specific messages for wrong arg counts, argument types, etc.

Doesn't need to hold up the landing of this patch, but at least file a follow-up bug. Might even make it a good first bug.

Other than that: r=me with feedback addressed.

::: js/src/builtin/SIMD.cpp
@@ +972,5 @@
>  }
>  
> +template<class VElem, unsigned NumElem>
> +static bool
> +CheckLoadStoreArgs(JSContext *cx, const CallArgs &args, int32_t *outByteStart, VElem **data)

Not a fan of the name: while this function does some of the args checking, its more important task is getting the typedArrayData ptr. Also, it really only checks the load args. "TypedArrayDataPtrFromArgs", perhaps?

byteStart isn't used in either of the callers, so remove it. After that, you could also have it return the pointer directly instead of using an outparam. Granted, that makes the nice "just return ErrorBadArgs(cx)" thing impossible, so your call.

@@ +977,5 @@
> +{
> +    if (!args[0].isObject())
> +        return ErrorBadArgs(cx);
> +
> +    JSObject &argobj = args[0].toObject();

"typedArrayObj"?

@@ +981,5 @@
> +    JSObject &argobj = args[0].toObject();
> +    if (!argobj.is<TypedArrayObject>())
> +        return ErrorBadArgs(cx);
> +
> +    Rooted<TypedArrayObject*> typedArray(cx, &argobj.as<TypedArrayObject>());

We probably should have a RootedTypedArrayObject. (We do actually - in IonCaches.cpp ...)

This is not the patch for that, though. But, rs=me if you want to add it in a patch this one would land after.

::: js/src/builtin/SIMD.h
@@ +1,1 @@
> + /* vim: set ts=8 sts=4 et sw=4 tw=99:

Are we getting rid of mode lines? (Not that I particularly care.)
Attachment #8515039 - Flags: review?(till) → review+
(In reply to Till Schneidereit [:till] from comment #5)
> Comment on attachment 8515039 [details] [diff] [review]
> Load / Store
> 
> Review of attachment 8515039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think better error messages for invalid arguments would be very nice. We
> have specific messages for wrong arg counts, argument types, etc.
> 
> Doesn't need to hold up the landing of this patch, but at least file a
> follow-up bug. Might even make it a good first bug.
Good idea! Will file a bug and mentor it myself.

> 
> Not a fan of the name: while this function does some of the args checking,
> its more important task is getting the typedArrayData ptr. Also, it really
> only checks the load args. "TypedArrayDataPtrFromArgs", perhaps?
Indeed, fixed it.

> byteStart isn't used in either of the callers, so remove it.
Good catch, thanks.

> After that, you
> could also have it return the pointer directly instead of using an outparam.
> Granted, that makes the nice "just return ErrorBadArgs(cx)" thing
> impossible, so your call.
If we do this and want to improve error messages, we'll lose the precision of the error message (are arguments of the wrong type or did we try an OOB read/write?), so I'll keep it like this for now.

> 
> @@ +977,5 @@
> > +{
> > +    if (!args[0].isObject())
> > +        return ErrorBadArgs(cx);
> > +
> > +    JSObject &argobj = args[0].toObject();
> 
> "typedArrayObj"?
Unless you have a strong opinion here, I'd keep the argobj, as it's not checked yet to be a typedArrayObj; then later the Rooted<TypedArrayObject*> variable is named typedArray, which sounds more consistent.

> We probably should have a RootedTypedArrayObject. (We do actually - in
> IonCaches.cpp ...)
> 
> This is not the patch for that, though. But, rs=me if you want to add it in
> a patch this one would land after.
> 
There are 5 references to Rooted<TypedArrayObject*> in the entire tree, according to DXR, plus the typedef in IonCaches.cpp seems unused. I'd rather delete the typedef in IonCaches.cpp and use the long form for the 5 remaining references, but it doesn't really matter.

> ::: js/src/builtin/SIMD.h
> @@ +1,1 @@
> > + /* vim: set ts=8 sts=4 et sw=4 tw=99:
> 
> Are we getting rid of mode lines? (Not that I particularly care.)

Nope, just a mistake of mine, thanks for noticing.
Blocks: 1099149
For what it's worth, I'm fighting against a MacOS-only bug, reproducible only on TBPL (can't reproduce with a loaner nor with Luke's computer). Seems to be related to the -1 out of bounds. Not even sure whether Mac OS X on tbpl is running 32 or 64 bits, got to dig in further in the logs.
The binaries are universal, but all of our OSX test slaves are 64bit.
https://hg.mozilla.org/mozilla-central/rev/0432a14c9283
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.