Closed Bug 1461376 Opened 2 years ago Closed Last year

Deduplicate emitLoadObject

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mgaudet, Assigned: bobslept, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1461375 +++

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::emitLoadObject and BaselineCacheIRCompiler::emitLoadObject 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


[1]: https://jandemooij.nl/blog/2017/01/25/cacheir/
[2]: https://en.wikipedia.org/wiki/Inline_caching
No longer depends on: 1461375
Building and running alright. Passes jsapi-test and cacheir jit_test. Did not ran a full jit_test yet.
Attachment #8977078 - Flags: review?(mgaudet)
Comment on attachment 8977078 [details] [diff] [review]
1461376-deduplicate-loadobject.patch

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

Looks good to me. One minor nit would be that I think it might be a good idea to try to match the order inside CacheIRCompiler.cpp to the enum of shared opcodes (which would put that earlier in the file). 

Having said that, I'd be equally OK with a followup bug aimed specifically at trying to do that. 

Pushed to try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c3b41c80bbcecdfc236ffbdd8ca20be904d7e0d
Attachment #8977078 - Flags: review?(mgaudet) → review+
Assignee: nobody → bobslept
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7aa8e8f9b0e
Deduplicate LoadObject using emitLoadStubField. r=mgaudet
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d7aa8e8f9b0e
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.