1348792 - Try to share more CacheIR compiler code

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Right now we can't share codegen of instructions that read stub fields because in Baseline they have to read from the stub data (because stub code is shared) but in Ion we bake in the stub fields directly.

Some options to consider:

* We could add methods to the CacheIRCompiler base class that return something like AddressOrConstant<T> (Address if Baseline, constant T if Ion).

* We could make Ion ICs read from the stub data, like Baseline. For many CacheIR ops this should be fine as it has negligible overhead compared to baking in the pointer.

Comment 1

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

Attachment: Demonstrate how loads of stub fields can be done in IonMonkey

Comment 2

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

Attachment: Very preliminary patch for sharing stubs with stub fields

This patch creates the AddressOrConstant struct and helper
EmitLoadAddressOrConstant that fills a register with a value, either by baking
a constant, or by creating an address load.

The patch demonstrates deduplication by eliminating the duplication of
emitMegamorphicLoadSlotResult caused by stub fields.

Comment 3

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

Attachment: (Prototype) Demonstrate how loads of stub fields can be done in IonMonkey

Comment 4

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

Attachment: [Part 1]: Lay down the infrastructure for sharing stubs with stub fields

Comment 5

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

Attachment: [Part 2 (Optional)] Convert infrastructure for sharing stub fields to templates

By doing so, we are able to have linking errors for un-handled StubField::Types, as opposed to runtime asserts.

This changeset is optional; the downside is an enlargement of footprint.

Comment 6

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

Attachment: [Part 3] Deduplicate MegamorphicLoadSlotResult using EmitLoadAddressOrConstant

Show how the infrastructure can reduce the amount of code required by deduplicating a complicated stub.

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 8946774 [details] [diff] [review]
[Part 1]: Lay down the infrastructure for sharing stubs with stub fields

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

(Two months late...)

I like the direction this is going. I'd prefer better separation of implementation (stub data vs constants) from policy (Ion uses constants).

::: js/src/jit/CacheIRCompiler.cpp
@@ +2643,5 @@
> + * MG:XXX: Currently we don't provide a mechanism to choose between a GC'd pointer
> + *         and ImmPtr; Do we need this functionality?
> + *
> + */
> +void CacheIRCompiler::EmitIonLoadConstant(AddressOrConstant val, Register dest) {

EmitLoadStubFieldConstant maybe?

@@ +2661,5 @@
> +/*
> + * After this is done executing, dest contains the value; either through a constant load
> + * or through the load from the stub data.
> + */
> +void CacheIRCompiler::EmitLoadAdddressOrConstant(AddressOrConstant val, Register dest) {

EmitLoadStubField maybe?

Add comments explaining Ion policy is to monomorphize ICs, while Baseline shares. Perhaps a flag could be added to CacheIRCompiler for this policy, and the constructor decides it based on mode_.

::: js/src/jit/CacheIRCompiler.h
@@ +515,5 @@
> +/**
> + * Wrap an offset so that a call can decide to embed a constant
> + * or load from the stub data.
> + */
> +class AddressOrConstant {

StubFieldOffset maybe?

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 8946775 [details] [diff] [review]
[Part 2 (Optional)] Convert infrastructure for sharing stub fields to templates

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

I'd favor boring code since the upside is small and the |default: MOZ_CRASH()| pattern is something we use everywhere in engine.

Comment 9

[Ted Campbell [:tcampbell]]

Comment on attachment 8946776 [details] [diff] [review]
[Part 3] Deduplicate MegamorphicLoadSlotResult using EmitLoadAddressOrConstant

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

Nice!

Comment 10

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

Attachment: Lay down the infrastructure for sharing stub emission code when stub fields are involved

I think this infrastructure is ready to land as is. 

I've opened Bug 1459038 to land deduplication patches: It would also be a good-second-bug sort of thing (in case I don't get there).

Comment 11

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

A try build shows an assertion failure (that looks legitimate) in one of the jsreftests.

Assertion failure: stubFields_[i].type() == type, at /builds/worker/workspace/build/src/js/src/jit/CacheIR.h:529
REFTEST PROCESS-CRASH | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-453024.js | application crashed [@ js::jit::CacheIRCompiler::readStubWord]

Here's the extracted backtrace: 

0  libxul.so!js::jit::CacheIRCompiler::readStubWord [CacheIR.h:caeda2f08587ec1a796e2e7bee4ee22cd19796da : 529 + 0x18]
   eip = 0xf1cb9b28   esp = 0xffbb8920   ebp = 0xffbb8928   ebx = 0xf544a000
   esi = 0x00000000   edi = 0xffbb8b08   eax = 0x00000000   ecx = 0x00000000
   edx = 0x00000000   efl = 0x00210246
   Found by: given as instruction pointer in context
1  libxul.so!js::jit::IonCacheIRCompiler::emitGuardDOMExpandoMissingOrGuardShape [CacheIRCompiler.h:caeda2f08587ec1a796e2e7bee4ee22cd19796da : 679 + 0x18]
   eip = 0xf1cf95d4   esp = 0xffbb8930   ebp = 0xffbb89b8   ebx = 0xffbb8af8
   esi = 0xffbb9610   edi = 0xffbb8b08
   Found by: call frame info
2  libxul.so!js::jit::IonCacheIRCompiler::compile [IonCacheIRCompiler.cpp:caeda2f08587ec1a796e2e7bee4ee22cd19796da : 553 + 0x78]
   eip = 0xf1d24b60   esp = 0xffbb89c0   ebp = 0xffbb8a78   ebx = 0xffbb8afc
   esi = 0xffbb8af8   edi = 0xf544a000
   Found by: call frame info
3  libxul.so!js::jit::IonIC::attachCacheIRStub [IonCacheIRCompiler.cpp:caeda2f08587ec1a796e2e7bee4ee22cd19796da : 2473 + 0xe]
   eip = 0xf1d25a02   esp = 0xffbb8a80   ebp = 0xffbb9a38   ebx = 0xda26d5f0
   esi = 0x007f7f7f   edi = 0xffbb8af8
   Found by: call frame info
4  libxul.so!js::jit::IonGetPropertyIC::update [IonIC.cpp:caeda2f08587ec1a796e2e7bee4ee22cd19796da : 148 + 0x35]
   eip = 0xf1d4f642   esp = 0xffbb9a40   ebp = 0xffbb9c28   ebx = 0xffbb9c64
   esi = 0xda2c5000   edi = 0xda2c51a0
   Found by: call frame info
5  0x47237c8e
   eip = 0x47237c8e   esp = 0xffbb9c30   ebp = 0xffffff8c   ebx = 0xffbb9c50
   esi = 0xe24fb850   edi = 0xe1fc7300
   Found by: call frame info

It is perhaps notable that it only reproduces on a 32 bit build. 

I've been so far entirely unable to reproduce. Bug 1391853 means --filter is broken, and if I just run the whole test suite I get a fatal error about  "non-local network connections". 

To attempt to narrow down, I've kicked off a new try build that consists of only the patch attached to this bug https://treeherder.mozilla.org/#/jobs?repo=try&revision=9007baeebfc9301347e9c2d88924e20ab6eca4db 

It could be that I'm just unlucky with the my choice of function to de-duplicate, in that it's only being triggered reliably through one of these reftests. Assuming the above try build is green, I'll try de-duplicating something more common.

Comment 12

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

Attachment: Lay down the infrastructure for sharing stub emission code when stub fields are involved

This rephrases the stub field code in terms of a policy type.

Comment 13

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

(Fixed the bug mentioned in Comment 11, it was rooted in a misunderstanding on my part)

Comment 14

[Ted Campbell [:tcampbell]]

Comment on attachment 8973820 [details] [diff] [review]
Lay down the infrastructure for sharing stub emission code when stub fields are involved

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

Great! I totally missed the stubDataOffset/nextStubField distinction as well =\

::: js/src/jit/CacheIRCompiler.h
@@ +579,5 @@
>      // Whether this IC may read double values from uint32 arrays.
>      mozilla::Maybe<bool> allowDoubleResult_;
>  
> +    // Distance from the IC to the stub data; mostly will be
> +    // sizeof(stubStype)

"stubStype" typo?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f122e99424f9
Lay down the infrastructure for sharing stub emission code when stub fields are involved r=tcampbell

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f122e99424f9