Deduplicate emitGuardGroupHasUnanalyzedNewScript

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mgaudet, Assigned: bobslept, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

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::emitGuardGroupHasUnanalyzedNewScript and BaselineCacheIRCompiler::emitGuardGroupHasUnanalyzedNewScript 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
Assignee

Comment 1

a year ago
Is this the correct approach? It's building and running alright. Also passing jsapi-test.
Comment on attachment 8976602 [details] [diff] [review]
1461372-deduplicate-guardgrouphasunanalyzednewscript.patch

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

Thanks so much! 

This looks really good. I'm marking r+, and I have just kicked off a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b3fa7c7e4b58fdc61c678e56e6c97a484bac5b5
Attachment #8976602 - Flags: review+
Assignee: nobody → bobslept
facepalm. I picked the commit onto an old revision. Here's a new try build with an up-to-date central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c31728e0eb27e63e07a0565e4c15ad0568887c1
Comment on attachment 8976602 [details] [diff] [review]
1461372-deduplicate-guardgrouphasunanalyzednewscript.patch

Turns out try caught a missed portion of the patch! Have talked with bobslept over IRC.
Attachment #8976602 - Flags: review+
Assignee

Comment 5

a year ago
Alright I have added the missing part in this patch.
Attachment #8976602 - Attachment is obsolete: true
Attachment #8976691 - Flags: review?(mgaudet)
A new try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81235cd075cec74d0130c5c3fd492bbe02cdf2c4

(Eyeballing the changes look good. I'll just sit on the r+ until the try is green)
Comment on attachment 8976691 [details] [diff] [review]
1461372-deduplicate-guardgrouphasunanalyzednewscript.patch

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

Great, thanks for the fix!
Attachment #8976691 - Flags: review?(mgaudet) → review+
Assignee

Updated

a year ago
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f17388afa985
Deduplicate GuardGroupHasUnanalyzedNewScript using emitLoadStubField. r=mgaudet
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f17388afa985
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.