Closed Bug 1461386 Opened 2 years ago Closed Last year

Deduplicate emitGuardFunctionPrototype

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: mgaudet, Assigned: wcosta, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

59 bytes, text/x-review-board-request
mgaudet
: review+
Details
+++ This bug was initially created as a clone of Bug #1461372 +++

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::emitGuardFunctionPrototype and BaselineCacheIRCompiler::emitGuardFunctionPrototype 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: nobody → wcosta
Status: NEW → ASSIGNED
Hi :) 

So, the trick here is going to be the function emitLoadStubField [1], and the wrapper type StubFieldOffset. 

What these two do is allow you to move a stub field value into an offset, following the policy defined by which compiler the stub is being generated for (Currently in baseline, it will load from the stub field data, to allow sharing stub -code- between different caches, but in Ion it will bake the value into the code as an immediate) 

So, what you'll want to do is wrap the stub field read in a StubFieldOffset, then use emitLoadStubField to replace the move32(Imm32()) or the load32 (whichever one you move into CacheIRCompiler.cpp

Then you'll need to register a the op as a 'shared op' by adding it to the list of CACHEIR_SHARED_OPS [2]


[1]: https://searchfox.org/mozilla-central/search?q=symbol:_ZN2js3jit15CacheIRCompiler17emitLoadStubFieldENS0_15StubFieldOffsetENS0_8RegisterE&redirect=false 
[2]: https://searchfox.org/mozilla-central/source/js/src/jit/CacheIRCompiler.h#19
Flags: needinfo?(mgaudet)
Comment on attachment 8995179 [details]
Bug 1461386: Deduplicate emitGuardFunctionPrototype

https://reviewboard.mozilla.org/r/259666/#review266670

Looking great so far. One more fix and I think it'll be good to go. 

Probably can also check and see if it's green on try; something like "try: -b d -p linux64,linux,win64 -u xpcshell,jittests,jittest-1,jittest-2,mochitests -t none" would be my suggestion

::: js/src/jit/CacheIRCompiler.cpp:1650
(Diff revision 1)
> +    if (!addFailurePath(&failure))
> +        return false;
> +
> +     // Guard on the .prototype object.
> +    masm.loadPtr(Address(obj, NativeObject::offsetOfSlots()), scratch1);
> +    emitLoadStubField(StubFieldOffset(reader.stubOffset(), StubField::Type::RawWord), scratch2);

I think for readability it would be nice to give the StubFieldOffset a name, the same way it was in the Ion compiler. 

I'd suggest something like:

```
StubFieldOffset slot(reader.stubOffset(), StubField::Type::RawWord)
masm.loadPtr(Address(obj, NativeObject::offsetOfSlots()), scratch1);
emitLoadStubField(slot, scratch2);
```
Attachment #8995179 - Flags: review?(mgaudet)
Comment on attachment 8995179 [details]
Bug 1461386: Deduplicate emitGuardFunctionPrototype

https://reviewboard.mozilla.org/r/259666/#review266730

Looks much better. 

I notice there's some failures on try. I have a pretty good intuition as to why: 

You'll need to add support for RawWord here: https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/js/src/jit/CacheIRCompiler.cpp#3210 

You'll use the same method the IonCacheIRCompiler method does: readStubWord

You can test locally by following the build and test instructions here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
Attachment #8995179 - Flags: review?(mgaudet)
Comment on attachment 8995179 [details]
Bug 1461386: Deduplicate emitGuardFunctionPrototype

https://reviewboard.mozilla.org/r/259666/#review267052

Looks good now! r+ with a couple of nits addressed :)

::: js/src/jit/CacheIRCompiler.cpp:3254
(Diff revisions 2 - 3)
>          masm.movePtr(ImmGCPtr(groupStubField(val.getOffset())), dest);
>          break;
>        case StubField::Type::JSObject:
>          masm.movePtr(ImmGCPtr(objectStubField(val.getOffset())), dest);
>          break;
> +      case StubField::Type::RawWord: {

I don't think we want these braces here

::: js/src/jit/CacheIRCompiler.cpp:3255
(Diff revisions 2 - 3)
>          break;
>        case StubField::Type::JSObject:
>          masm.movePtr(ImmGCPtr(objectStubField(val.getOffset())), dest);
>          break;
> +      case StubField::Type::RawWord: {
> +        auto slot = readStubWord(val.getOffset(), StubField::Type::RawWord);

I believe the spidermonkey style here wouldn't use auto, instead preferring to specify the type.

I think the better answer here would be to fold it all into one move32 expression, and just break the line.
Attachment #8995179 - Flags: review?(mgaudet) → review+
Comment on attachment 8995588 [details]
test

https://reviewboard.mozilla.org/r/259998/#review267080

::: js/src/jit/CacheIRCompiler.cpp:3426
(Diff revision 1)
>          masm.movePtr(ImmGCPtr(objectStubField(val.getOffset())), dest);
>          break;
> -      case StubField::Type::RawWord: {
> -        auto slot = readStubWord(val.getOffset(), StubField::Type::RawWord);
> -        masm.move32(Imm32(slot), dest);
> +      case StubField::Type::RawWord:
> +        masm.move32(
> +            Imm32(readStubWord(val.getOffset(), StubField::Type::RawWord)),
> +            dest

I think a more stylistically harmonious way to do this would be 

```
masm.move32(Imm32(readStubWord(val.getOffset(), StubField::Type::RawWord)),
            dest); 
```
Attachment #8995588 - Attachment is obsolete: true
Ok to land this?
Flags: needinfo?(mgaudet)
Yep, I think it's good to go.
Flags: needinfo?(mgaudet)
Pushed by wcosta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8775ded16701
Deduplicate emitGuardFunctionPrototype r=mgaudet
https://hg.mozilla.org/mozilla-central/rev/8775ded16701
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.