Closed
Bug 1460895
Opened 6 years ago
Closed 6 years ago
Deduplicate emitMegamorphicLoadSlotResult
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: mgaudet, Assigned: mgaudet, Mentored)
References
Details
Attachments
(1 file)
7.06 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
CacheIR [1] is a system for emitting Inline Cache [2] stubs in a rigorous manner. Occasionally a stub needs to refer to data that is specific to the particular stub. This is a accomplished with StubFields in CacheIR. Before Bug 1348792, the policy for handling stub fields was implicit, a property of the implementation: Ion would bake values into stubs, and baseline wouldn't (allowing the sharing of machine code). This bug is about taking advantage of the new machinery added in Bug 1348792, which allows us to use the same CacheIR code generation code for Ion and Baseline where the only difference between stubs is the handling of StubFields. The function IonCacheIRCompiler::emitMegamorphicLoadSlotResult and BaselineCacheIRCompiler::emitMegamorphicLoadSlotResult are almost identical, except in their handling of stub-fields, and therefore can be shared. Tasks: * Verify that the two compiler functions remain almost identical except for their handling of stubFields * Unify the implementations by moving it into CacheIRCompiler.cpp, adding the opcode to the CACHE_IR_SHARED_OPS list, and modifying the stub field loading to use a StubFieldOffset and EmitLoadStubField. * Delete the old implementations * Ensure all the tests still pass
Assignee | ||
Comment 1•6 years ago
|
||
(oops, missed footnotes) [1]: https://jandemooij.nl/blog/2017/01/25/cacheir/ [2]: https://en.wikipedia.org/wiki/Inline_caching
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8975039 -
Flags: review?(jdemooij)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mgaudet
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•6 years ago
|
||
Jan: This bug looks weird, because there's all kinsd of info on it, but i'm planning on using it as a template for a bunch of mentorable-good-first-bugs based on this work, assuming you're cool with this approach and patch.
Comment 4•6 years ago
|
||
Comment on attachment 8975039 [details] [diff] [review] Deduplicate MegamorphicLoadSlotResult using EmitLoadStubField Review of attachment 8975039 [details] [diff] [review]: ----------------------------------------------------------------- Ah, nice! ::: js/src/jit/CacheIRCompiler.cpp @@ +3047,5 @@ > + masm.setupUnalignedABICall(scratch1); > + masm.loadJSContext(scratch1); > + masm.passABIArg(scratch1); > + masm.passABIArg(obj); > + EmitLoadStubField(name, scratch2); Pre-existing nit: these are class methods so could you rename s/Emit/emit/? In SpiderMonkey code EmitLoadStubField suggests it's a stand-alone function.
Attachment #8975039 -
Flags: review?(jdemooij) → review+
Pushed by mgaudet@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb850fd1688 Deduplicate MegamorphicLoadSlotResult using EmitLoadStubField r=jandem
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9eb850fd1688
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•