Closed Bug 1169214 Opened 5 years ago Closed 4 years ago

[MBaselineCache] Adjust JitStubHelpers to have ionmonkey variants

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(4 files, 1 obsolete file)

JitStubHelper has callVM/TailCallVM/EnterExitFrame/... helpers. Those are currently specialised for Baseline. We need similar function for IonMonkey.
Blocks: 1161516
Assignee: nobody → hv1989
Attachment #8621587 - Flags: review?(jdemooij)
Attachment #8621588 - Flags: review?(jdemooij)
Simplified the non-tail calls a bit.
Attachment #8621588 - Attachment is obsolete: true
Attachment #8621588 - Flags: review?(jdemooij)
Attachment #8621606 - Flags: review?(jdemooij)
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.
Attachment #8621587 - Flags: review?(jdemooij) → review+
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/
Attachment #8621606 - Flags: review?(jdemooij) → review+
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 :)
Attachment #8621608 - Flags: review?(jdemooij) → review+
This patchset forgot about (and consequently broke) ARM64!
Follow-up patch to fix ARM64 build.
Attachment #8651240 - Flags: review?(hv1989)
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?
Attachment #8651240 - Flags: review?(hv1989) → review+
See Also: → 1203287
You need to log in before you can comment on or make changes to this bug.