Try to share more CacheIR compiler code

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jandem, Assigned: mgaudet)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [qf-])

Attachments

(2 attachments, 6 obsolete attachments)

4.84 KB, patch
Details | Diff | Splinter Review
11.34 KB, patch
tcampbell
: review+
Details | Diff | Splinter Review
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.
Whiteboard: [qf]
Whiteboard: [qf] → [qf-]
Assignee: nobody → mgaudet
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.
Attachment #8943738 - Flags: review?(tcampbell) → feedback?(tcampbell)
Attachment #8943739 - Flags: review?(tcampbell) → feedback?(tcampbell)
Attachment #8943738 - Attachment is obsolete: true
Attachment #8943738 - Flags: feedback?(tcampbell)
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.
Attachment #8946775 - Flags: feedback?(tcampbell)
Show how the infrastructure can reduce the amount of code required by deduplicating a complicated stub.
Attachment #8943739 - Attachment description: Demonstrate how loads of stub fields can be done in IonMonkey → (Prototype) Demonstrate how loads of stub fields can be done in IonMonkey
Attachment #8943739 - Flags: feedback?(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?
Attachment #8946774 - Flags: feedback?(tcampbell) → feedback+
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.
Attachment #8946775 - Flags: feedback?(tcampbell) → feedback-
Comment on attachment 8946776 [details] [diff] [review]
[Part 3] Deduplicate MegamorphicLoadSlotResult using EmitLoadAddressOrConstant

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

Nice!
Attachment #8946776 - Flags: feedback+
Attachment #8946776 - Flags: feedback+
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).
Attachment #8973032 - Flags: review?(tcampbell)
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.
Attachment #8973032 - Attachment is obsolete: true
Attachment #8973032 - Flags: review?(tcampbell)
(Fixed the bug mentioned in Comment 11, it was rooted in a misunderstanding on my part)
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?
Attachment #8973820 - Flags: review?(tcampbell) → review+
Pushed by mgaudet@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/f122e99424f9
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.