Closed Bug 1461375 Opened Last year Closed Last year

Deduplicate emitMegamorphicStoreSlotResult


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




Tracking Status
firefox62 --- fixed


(Reporter: mgaudet, Assigned: simonrbrand, Mentored)



(Keywords: good-first-bug)


(1 file, 2 obsolete files)

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

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::emitMegamorphicStoreSlotResult and BaselineCacheIRCompiler::emitMegamorphicStoreSlotResult are almost identical, except in their handling of stub-fields, and therefore can be shared. 


* 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

Blocks: 1461376
No longer blocks: 1461376
Attached patch megamorphic.patch (obsolete) — Splinter Review
First time contributing! I followed the instructions as written in the bug. I ran the jit-test suite, all of which passed. I then added an unconditional crash to the beginning of the function, as suggested on IRC, and noted that at least baseline/getter_setter.js now failed, so the function is covered.
Attachment #8983966 - Flags: review?(mgaudet)
Curious: Your patch seems to have no attached commit message. (Applies cleanly tho)

One of the best ways to upload a patch is to use |hg bzexport <rev>|, which will upload it directly to bugzilla if you have your API key setup. Otherwise, |hg export <rev>| will do a local export with commit info.

I think patches exported by git format-patch are also OK, if you're using a git mirror (I don't have personal experience). 

Once we get that taken care of, I think I can r+ this patch, it looks good.

In the mean time, here's a try build we can use to check and make sure:
Assignee: nobody → simonrbrand
Patch looks good, but I think it'd be worth having a version uploaded with commit message (so that it can be landed by setting the checkin-needed flag).
Attachment #8983966 - Flags: review?(mgaudet)
Attached patch megamorphic-commit.patch (obsolete) — Splinter Review
Thanks for reviewing! The above patch should also have a commit message.
Comment on attachment 8984290 [details] [diff] [review]

Review of attachment 8984290 [details] [diff] [review]:

Mostly good, just one nit before I can r+. 

When you upload a new patch, you can obsolete the old ones. Will make using the checkin-needed keyword easier.

::: js/src/jit/CacheIRCompiler.cpp
@@ +3123,5 @@
>  bool
> +CacheIRCompiler::emitMegamorphicStoreSlot()
> +{
> +    MOZ_CRASH("hit");

Remove this :)
Whoops! Third time's a charm I guess.
Attachment #8983966 - Attachment is obsolete: true
Attachment #8984290 - Attachment is obsolete: true
Comment on attachment 8984559 [details] [diff] [review]

Yep. Looks good to me! 

So at this point, I believe you should be able to add the keyword 'checkin-needed' to the bug, and a sheriff will land the code. (If you can't, ping me on IRC or leave another comment on the bug)

Thanks so much for doing this, and I you'll contribute more :D
Attachment #8984559 - Flags: review+
Keywords: checkin-needed
Pushed by
Deduplicate emitMegamorphicStoreSlotResult between Ion and Baseline. r=mgaudet
Keywords: checkin-needed
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.