1337024 - Convert SETELEM dense/typed array IC to CacheIR

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Tom S]

The dense/typed array SETELEM code is not yet ported to CacheIR and duplicated (differently) in Ion and Baseline.

We should make sure the dense element stubs work for all objects with dense elements and not just Arrays!

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Setting dense/unboxed elements

Pretty straight-forward. The old code did too many things which makes it hard to understand. Splitting it up like this makes it much more readable.

That said, the stub for *adding* elements is the scary one, but one step at a time :)

Comment 2

[Tom S]

Comment on attachment 8835472 [details] [diff] [review]
Part 1 - Setting dense/unboxed elements

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

Much better, but this was the simpler part.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1055,5 @@
> +    // We need to convert int32 values being stored into doubles. Note that
> +    // double arrays are only created by IonMonkey, so if we have no FP support
> +    // Ion is disabled and there should be no double arrays.
> +    if (cx_->runtime()->jitSupportsFloatingPoint)
> +        masm.convertInt32ValueToDouble(val);

From what I understand this is fine in Baseline, but changing the type like this can be tricky.

::: js/src/jit/BaselineInspector.h
@@ -36,5 @@
>      { }
>  
>      bool sawOOBDenseWrite() const;
>      bool sawOOBTypedArrayWrite() const;
> -    bool sawDenseWrite() const;

Wasn't even used ..

::: js/src/jit/CacheIR.cpp
@@ +2243,5 @@
> +    if (!obj->isNative())
> +        return false;
> +
> +    NativeObject* nobj = &obj->as<NativeObject>();
> +    if (!nobj->containsDenseElement(index) || nobj->getElementsHeader()->isFrozen())

Amazing how much simpler this when not handling this case after the SetElem call and not conflating the add/set case.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #2)
> From what I understand this is fine in Baseline, but changing the type like
> this can be tricky.

Yes, I'll add a comment mentioning this is fine in Baseline but not for Ion.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Setting Dense/UnboxedArray holes

Removes a few hundred lines of code, and optimizes more things than before:

* We now handle setting holes at indexes < initLength.

* When we do JSOP_INITELEM_ARRAY etc we no longer guard on the prototype shapes as these objects are not relevant.

Comment 5

[Jan de Mooij [:jandem]]

Oh and --unboxed-arrays doesn't even pass jit-tests. I verified the new code works with some simple tests but if I had to start over porting stubs to CacheIR I'd probably have rmeoved the unboxed array code :/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96454c1e87ee
part 1 - Convert Baseline stubs for setting dense/unboxed elements to CacheIR. r=evilpie

Comment 7

[Jan de Mooij [:jandem]]

Part 1 was an unexpected 5.5-8.5% win on sunspider string-tagcloud on AWFY.

The win comes from the SetElem here:

    for (i in v) {
        ...
        v[i] = n;
    }

i is an integer-string and we didn't optimize that case before but maybeGuardInt32Index handles it fine \o/. There might be another win when we convert the Ion IC to CacheIR.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/96454c1e87ee

Comment 9

[Tom S]

Comment on attachment 8836025 [details] [diff] [review]
Part 2 - Setting Dense/UnboxedArray holes

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

Nice work! I am sure handling in-bounds sets is going to be a win somewhere. The whole disabled forever situation for TypedObjects and even more so for unboxed-arrays is really starting to irritate me as well.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1170,5 @@
> +    Label doStore;
> +    if (handleAdd) {
> +        // If index == initLength, increment initLength.
> +        Label inBounds;
> +        masm.branch32(Assembler::NotEqual, initLength, index, &inBounds);

So basically the handleAdd case can also handle the setting a hole case. It makes sense, but took me a while to understand that.

::: js/src/jit/CacheIR.cpp
@@ +2274,5 @@
> +            (clasp->getAddProperty() ||
> +             clasp->getResolve() ||
> +             clasp->getOpsLookupProperty() ||
> +             clasp->getSetProperty() ||
> +             clasp->getOpsSetProperty()))

Am I missing something or did we just not do this before?

@@ +2370,5 @@
> +
> +    // Type inference uses JSID_VOID for the element types.
> +    setUpdateStubInfo(nobj->group(), JSID_VOID);
> +
> +    trackAttached("StoreDenseElementHole");

Would be nice to nice to log something else for |isAdd|, but not crucial.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #9)
> Am I missing something or did we just not do this before?

We didn't. At least some of these hooks are not used at all or are not used for dense elements, and attaching the stub after the SetElement call probably helped avoid some problems with lookup/resolve hooks. I added the checks just to be safe.

> Would be nice to nice to log something else for |isAdd|, but not crucial.

I'll change it to: isAdd ? "AddDenseElement" : "StoreDenseElementHole"

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f60b9d2c5bd
part 2 - Port Baseline stubs for adding dense/unboxed elements to CacheIR. r=evilpie

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f60b9d2c5bd

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Setting TypedArray/TypedObject elements

We used to set a flag on the typed array stub if the access was out-of-bounds (this was added for a broken benchmark that was fixed since then I think), I changed that to set the flag on the fallback stub. I also renamed the hasArrayWriteHole flag to hasDenseAdd.

It might be nicer to add separate CacheIR instructions for the RHS double/int32 check/conversion and for the uint8 clamping, so we can share more code with Ion maybe, but we can always do that later when we know what the Ion code for this looks like.

This was the last Baseline SetProp/SetElem stub so we can finally start working on the parts we need for Ion.

Comment 14

[Jan de Mooij [:jandem]]

Oh and we also used to unlink non-OOB typed array stubs when we attached an OOB stub. I don't think we need to keep that behavior as OOB should be pretty rare for typed arrays and even if it happens we just have an extra stub but that's not too bad.

Comment 15

[Tom S]

Comment on attachment 8837569 [details] [diff] [review]
Part 3 - Setting TypedArray/TypedObject elements

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

\o/ And we are done porting Baseline GetProp/SetProp. 

I wonder if any benchmark we track actually hits the TypedArray OOB case, because Chakra and JSC supposedly throw on that.

::: js/src/jit/BaselineIC.cpp
@@ +1099,2 @@
>  void
> +BaselineStoreToTypedArray(JSContext* cx, MacroAssembler& masm, Scalar::Type type,

Can't we move this?

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #15)
> I wonder if any benchmark we track actually hits the TypedArray OOB case,
> because Chakra and JSC supposedly throw on that.

Good point, I can remove it if you want? It would simplify this patch quite a bit.

> ::: js/src/jit/BaselineIC.cpp
> > +BaselineStoreToTypedArray(JSContext* cx, MacroAssembler& masm, Scalar::Type type,
> 
> Can't we move this?

Yes. I didn't do that here to make it easier to review, but at some point we should reorganize the different IC files.

Comment 17

[Jan de Mooij [:jandem]]

We discussed this on IRC and we'll keep this for now because both Chakra and JSC don't throw on OOB sets.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98eeda9744e6
part 3 - Port Baseline TypedArray/TypedObject SetElem stub to CacheIR. r=evilpie

Comment 19

[Hannes Verschore [:h4writer]]

Little bit late, but marked this p1.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98eeda9744e6