1102510 - Add baseline ICs for typed object getprop and setprop

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

The attached patch adds support for baseline ICs that access plain scalar or reference properties of a struct typed object.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8526342 [details] [diff] [review]
patch

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

::: js/src/jit/BaselineIC.cpp
@@ +1560,5 @@
> +        id = NameToId(script->getName(pc));
> +        if (stub->toSetProp_TypedObject()->isObjectReference()) {
> +            // Ignore all values being written except plain objects. Null
> +            // is included implicitly in type information for this property,
> +            // and non-object non-null values will cause a bailout shortly.

I don't understand the bailout part, is it just that Ion does not need to know about the extra types because it will just bailout anyway?

@@ +5665,5 @@
> +            masm.storeToTypedFloatArray(type, ScratchFloat32Reg, dest);
> +        } else {
> +            masm.storeToTypedFloatArray(type, FloatReg0, dest);
> +        }
> +        masm.jump(&done);

Remove this jump, as the label is bound immediately after the if-else.

::: js/src/jit/BaselineIC.h
@@ +4606,5 @@
> +static uint8_t
> +SimpleTypeDescrKey(SimpleTypeDescr *descr)
> +{
> +    if (descr->is<ScalarTypeDescr>())
> +        return uint8_t(descr->as<ScalarTypeDescr>().type()) << 1;

It'd be good to either assert:

MOZ_ASSERT((uint8_t(type) << 1) == type);

Or a static_assert that the max value < 128 or something.

@@ +4607,5 @@
> +SimpleTypeDescrKey(SimpleTypeDescr *descr)
> +{
> +    if (descr->is<ScalarTypeDescr>())
> +        return uint8_t(descr->as<ScalarTypeDescr>().type()) << 1;
> +    return (uint8_t(descr->as<ReferenceTypeDescr>().type()) << 1) | 1;

And here.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> It'd be good to either assert:
> 
> MOZ_ASSERT((uint8_t(type) << 1) == type);

Er, this obviously won't work but you get the idea.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> ::: js/src/jit/BaselineIC.cpp
> @@ +1560,5 @@
> > +        id = NameToId(script->getName(pc));
> > +        if (stub->toSetProp_TypedObject()->isObjectReference()) {
> > +            // Ignore all values being written except plain objects. Null
> > +            // is included implicitly in type information for this property,
> > +            // and non-object non-null values will cause a bailout shortly.
> 
> I don't understand the bailout part, is it just that Ion does not need to
> know about the extra types because it will just bailout anyway?

"bailout" isn't the right word here (I'll fix this), but the baseline IC will not match when writing non-object non-null values to object references in a typed object, and we'll end up doing the assignment in the VM.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c20d396e45b3

Comment 5

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/926e13c29c4b for apparently breaking pretty much everything.

https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=c20d396e45b3

The previous pushes were all mostly green.

Comment 6

[Brian Hackett [Laid off!]]

Sorry, a test I changed right before pushing had a flipped condition.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9439c9dbd36e

Comment 7

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/mozilla-central/rev/9439c9dbd36e