Closed
Bug 1359952
Opened 7 years ago
Closed 7 years ago
[JITs] Unify InIRGenerator and HasOwnIRGenerator
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
People
(Reporter: tcampbell, Assigned: tcampbell)
References
Details
Attachments
(4 files)
We should combine the InIRGenerator (bug 1334187) and HasOwnIRGenerator (bug 1344469) since they are very similar. This should bring them up to feature parity.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → tcampbell
|
|
Assignee | |
Comment 5•7 years ago
|
||
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cbf0ace32ac95221640e91882ec42b7eb176d9c (The mozreview one somehow broke Gecko Decision Task..)
|
||
Comment 6•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8863384 [details] Bug 1359952 - Fix CacheIRCompiler handling of boolean results https://reviewboard.mozilla.org/r/135144/#review138456
Attachment #8863384 -
Flags: review?(jdemooij) → review+
|
|
||
Comment 7•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8863385 [details] Bug 1359952 - Remove shape arg from TestMatchingReceiver https://reviewboard.mozilla.org/r/135146/#review138458 Nice.
Attachment #8863385 -
Flags: review?(jdemooij) → review+
|
|
||
Comment 8•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8863386 [details] Bug 1359952 - Add ownProp flag to CanAttachDenseElementHole https://reviewboard.mozilla.org/r/135148/#review138460
Attachment #8863386 -
Flags: review?(jdemooij) → review+
|
|
||
Comment 9•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8863387 [details] Bug 1359952 - Add HasPropIRGenerator https://reviewboard.mozilla.org/r/135150/#review138462 Thanks, very nice! ::: js/src/jit/CacheIR.cpp:443 (Diff revision 1) > writer.guardShape(protoId, proto->as<NativeObject>().lastProperty()); > proto = proto->staticPrototype(); > lastObjId = protoId; > } > } > - } else if (obj->is<UnboxedPlainObject>()) { > + } else if (obj->is<UnboxedPlainObject>() && expandoId.isSome()) { I think I'd prefer adding an expicit HasPropIRGenerator::tryAttachUnboxed and using that instead of tryAttachNative.. Then we don't need this change right? Follow-up patch is fine. ::: js/src/jit/CacheIR.cpp:2095 (Diff revision 1) > - if (mode_ == ICState::Mode::Megamorphic) { > + // For HasOwn, we don't need to guard prototypes. Set holder to object > + // even though property is not found to avoid guarding the protochain. > + holder = obj; It may be more explicit to do this: if (hasOwn) { TestMatchingReceiver... } else { EmitReadSlotGuard... } ::: js/src/jit/CacheIR.cpp:2158 (Diff revision 1) > RootedObject obj(cx_, &val_.toObject()); > - > ObjOperandId objId = writer.guardIsObject(valId); > > - if (tryAttachProxyElement(keyId, obj, objId)) > + // Optimize DOM Proxies for JSOP_HASOWN > + if (cacheKind_ == CacheKind::HasOwn) Nit: add {} to the outer if-statement. Also please file a follow-up bug to support the megamorphic and proxy cases for JSOP_IN, maybe we can use templates for the VM functions.
Attachment #8863387 -
Flags: review?(jdemooij) → review+
|
|
||
Comment 10•7 years ago
|
||
Comment on attachment 8863387 [details] Bug 1359952 - Add HasPropIRGenerator Maybe evilpie should look at this too in case I missed something.
Attachment #8863387 -
Flags: feedback?(evilpies)
|
||
Comment 11•7 years ago
|
||
Comment on attachment 8863387 [details] Bug 1359952 - Add HasPropIRGenerator Looks good. I remember there was some problem with EmitReadSlotGuard for HasOwn, that's probably the expandoId you did.
Attachment #8863387 -
Flags: feedback?(evilpies) → feedback+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 16•7 years ago
|
||
TODO: Refactor the UnboxedObject / TypedObject cases to be more explicit / documented. They should work as is now, but it is not obvious that the special concerns are not handled. Also remove unnecessary guards described in https://bugzilla.mozilla.org/show_bug.cgi?id=1344469#c38.
Keywords: leave-open
| Comment hidden (mozreview-request) |
|
||
Comment 18•7 years ago
|
||
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/522b7ef5bd11 Fix CacheIRCompiler handling of boolean results r=jandem https://hg.mozilla.org/integration/autoland/rev/b33d51a80290 Remove shape arg from TestMatchingReceiver r=jandem https://hg.mozilla.org/integration/autoland/rev/a6ce36227095 Add ownProp flag to CanAttachDenseElementHole r=jandem https://hg.mozilla.org/integration/autoland/rev/c06620d52d5f Add HasPropIRGenerator r=jandem
|
||
Comment 19•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/522b7ef5bd11 https://hg.mozilla.org/mozilla-central/rev/b33d51a80290 https://hg.mozilla.org/mozilla-central/rev/a6ce36227095 https://hg.mozilla.org/mozilla-central/rev/c06620d52d5f
|
|
Assignee | |
Updated•7 years ago
|
Priority: -- → P3
|
|
Assignee | |
Comment 20•7 years ago
|
||
Closing bug as follow-up is now part of Bug 1357759.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
|
||
Comment 21•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•