1169214 - [MBaselineCache] Adjust JitStubHelpers to have ionmonkey variants

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

JitStubHelper has callVM/TailCallVM/EnterExitFrame/... helpers. Those are currently specialised for Baseline. We need similar function for IonMonkey.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Create Ion shared stub helpers

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Add the x86 shared stub helpers

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Add the x86 shared stub helpers

Simplified the non-tail calls a bit.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Part 3: Add the x64 shared stub helpers

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8621587 [details] [diff] [review]
Part 1: Create Ion shared stub helpers

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

::: js/src/jit/SharedIC.cpp
@@ +740,5 @@
>  
>  void
>  ICStubCompiler::enterStubFrame(MacroAssembler& masm, Register scratch)
>  {
> +    if (engine_ == Engine::Baseline) {

Nit: no {}

@@ +760,5 @@
>  {
>      MOZ_ASSERT(entersStubFrame_ && inStubFrame_);
>      inStubFrame_ = false;
> +
> +    if (engine_ == Engine::Baseline) {

And here.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8621606 [details] [diff] [review]
Part 2: Add the x86 shared stub helpers

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

::: js/src/jit/x86/SharedICHelpers-x86.h
@@ +129,5 @@
>  inline void
>  EmitIonCallVM(JitCode* target, size_t stackSlots, MacroAssembler& masm)
>  {
> +    // FIXME: #1169214: Make sure all stubcode use push/pop calls that
> +    // adjust the frame pushed state.

Nit: I think we can remove the comment; if a stub does not update the frame pushed state it's a bad crash/bug, not just an optional FIXME.

@@ +136,5 @@
> +    masm.call(target);
> +
> +    // Remove rest of the frame left on the stack. We remove the return address
> +    // which is implicitly poped when returning.
> +    int framePop = sizeof(ExitFrameLayout) - sizeof(void*);

Nit: s/int/size_t/

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8621608 [details] [diff] [review]
Part 3: Add the x64 shared stub helpers

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

See nits for the previous patch :)

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e93437f8667
https://hg.mozilla.org/integration/mozilla-inbound/rev/39305ec41209
https://hg.mozilla.org/integration/mozilla-inbound/rev/590916330cee

Comment 9

[Sean Stangl [:sstangl]]

This patchset forgot about (and consequently broke) ARM64!

Comment 10

[Sean Stangl [:sstangl]]

Attachment: Follow-up to port the patch to ARM64.

Follow-up patch to fix ARM64 build.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e93437f8667
https://hg.mozilla.org/mozilla-central/rev/39305ec41209
https://hg.mozilla.org/mozilla-central/rev/590916330cee

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8651240 [details] [diff] [review]
Follow-up to port the patch to ARM64.

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

Don't we have treeherder arm64 builds? (even if only builds and no tests?). It took me a while to get it green. 
So I was happy it was finally green and as a result I pushed.

In order to not have this anymore. Do we have simulator so I can build and test locally?

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20914863e9b5

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20914863e9b5