Closed
Bug 1102510
Opened 10 years ago
Closed 9 years ago
Add baseline ICs for typed object getprop and setprop
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(1 file)
36.90 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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 1•9 years ago
|
||
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+
Comment 2•9 years ago
|
||
(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•9 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.
Assignee | ||
Comment 4•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c20d396e45b3
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•9 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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9439c9dbd36e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Assignee: nobody → bhackett1024
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•