1461386 - Deduplicate emitGuardFunctionPrototype

Status: RESOLVED FIXED

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

Description

[Matthew Gaudet (he/him) [:mgaudet]]

+++ 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

Comment 1

[Wander Lairson Costa]

Looking at both methods [1,2]

They only differ by these two lines [3]. I am stuck in how the machinery of CacheIR works to unify them.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/BaselineCacheIRCompiler.cpp#477
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonCacheIRCompiler.cpp#869
[3] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonCacheIRCompiler.cpp#885-886

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

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

Comment 3

[Wander Lairson Costa]

Attachment: Bug 1461386: Deduplicate emitGuardFunctionPrototype

Move emitGuardFunctionPrototype to CacheIRCompiler and remove it from
both Baseline and Ion.

Review commit: https://reviewboard.mozilla.org/r/259666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259666/

Comment 4

[Matthew Gaudet (he/him) [: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);
```

Comment 5

[Wander Lairson Costa]

Comment on attachment 8995179 [details]
Bug 1461386: Deduplicate emitGuardFunctionPrototype

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259666/diff/1-2/

Comment 6

[Matthew Gaudet (he/him) [: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

Comment 7

[Wander Lairson Costa]

Comment on attachment 8995179 [details]
Bug 1461386: Deduplicate emitGuardFunctionPrototype

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259666/diff/2-3/

Comment 8

[Matthew Gaudet (he/him) [: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.

Comment 9

[Wander Lairson Costa]

Attachment: test

Review commit: https://reviewboard.mozilla.org/r/259998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259998/

Comment 10

[Wander Lairson Costa]

Comment on attachment 8995179 [details]
Bug 1461386: Deduplicate emitGuardFunctionPrototype

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259666/diff/3-4/

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

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); 
```

Comment 12

[Wander Lairson Costa]

Comment on attachment 8995179 [details]
Bug 1461386: Deduplicate emitGuardFunctionPrototype

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259666/diff/4-5/

Comment 13

[Wander Lairson Costa]

Ok to land this?

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

Yep, I think it's good to go.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8775ded16701
Deduplicate emitGuardFunctionPrototype r=mgaudet

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/8775ded16701