Add baseline ICs for typed object getprop and setprop

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla36
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8526342 [details] [diff] [review]
patch

The attached patch adds support for baseline ICs that access plain scalar or reference properties of a struct typed object.
Attachment #8526342 - Flags: review?(jdemooij)
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.
Attachment #8526342 - Flags: review?(jdemooij) → review+
(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.
(Assignee)

Comment 3

3 years ago
(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.
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.
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9439c9dbd36e
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/mozilla-central/rev/9439c9dbd36e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Assignee: nobody → bhackett1024
Target Milestone: --- → mozilla36
Depends on: 1247862
You need to log in before you can comment on or make changes to this bug.