Closed Bug 835277 Opened 7 years ago Closed 7 years ago

BaselineCompiler: Compile eval frames

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(4 files, 3 obsolete files)

We want to compile (at least) the same scripts JM compiles, so that means we have to support eval frames.
Attached patch WIP (obsolete) — Splinter Review
Passes jit-tests on x86.

The debugger tests use eval extensively, so this patch also fixes some small debugger issues.
Patch for inbound, pretty straight-forward.
Attachment #707067 - Flags: review?(kvijayan)
The baseline compiler uses script->hasBreakpointsAt(pc) when toggling breakpoints/traps in the script. We can call this function (via destroy -> dec -> recompile) when enabledCount == 0 but the BreakpointSite is not yet destroyed.

Not really related to eval, but this fixes 2 failures introduced by other patches in this bug, so I'm just attaching it here.
Attachment #707073 - Flags: review?(bhackett1024)
Attachment #707067 - Flags: review?(kvijayan) → review+
Attachment #707102 - Flags: review?(nicolas.b.pierron)
Attachment #707073 - Flags: review?(bhackett1024) → review+
This patch depends on the other patches in this bug (I will land these on mozilla-inbound first).
Attachment #707029 - Attachment is obsolete: true
Attachment #707588 - Flags: review?(kvijayan)
Inline IonFrameIterator::calleeToken() as well.
Attachment #707102 - Attachment is obsolete: true
Attachment #707102 - Flags: review?(nicolas.b.pierron)
Attachment #707593 - Flags: review?(nicolas.b.pierron)
Comment on attachment 707588 [details] [diff] [review]
Part 4: Compile eval scripts

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

Nice and clean and very understandable code :)

::: js/src/ion/BaselineJIT.cpp
@@ +160,2 @@
>          // Single transition point from Interpreter to Ion.
> +        enter(jitcode, maxArgc, maxArgv, fp, calleeToken, evalScopeChain, &result);

Nit: fix comment above.  "from Interpreter to Baseline" instead of "from Interpreter to Ion"

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +168,5 @@
>      masm.finishDataTransfer();
>  
> +    if (type == EnterJitBaseline) {
> +        JS_ASSERT(R1.scratchReg() != r0);
> +        masm.ma_mov(slot_evalScopeChain, R1.scratchReg());

Can't use the slot_evalScopeChain here, since it's based off of |sp|, and |sp| is modified by the data transfer that occurs before this code (they're effectively hidden pushes).

Since all the relevant registers are saved by the end of the first data transfer, you may want to take a register at that point, save sp, and use use that for the address that refers to the eval scope chain.

Small side note: the naming of the fields in register fields in EnterJITStack is in the wrong order.  |lr| should be the first field and |r0| should be the last field before |token| and |vp|.  This is not your bug though.
Attachment #707588 - Flags: review?(kvijayan) → review+
Inlining script() is annoying because it requires including IonFrames-inl.h in more files on some platforms. I don't think it's necessary so this just calls script().
Attachment #707593 - Attachment is obsolete: true
Attachment #707593 - Flags: review?(nicolas.b.pierron)
Attachment #708063 - Flags: review?(nicolas.b.pierron)
Attachment #708063 - Flags: review?(nicolas.b.pierron) → review+
Whiteboard: [leave open]
https://hg.mozilla.org/projects/ionmonkey/rev/6a7f35be6722

(In reply to Kannan Vijayan [:djvj] from comment #7)
> Can't use the slot_evalScopeChain here, since it's based off of |sp|, and
> |sp| is modified by the data transfer that occurs before this code (they're
> effectively hidden pushes).

Oops, good catch! Fixed by using r11.
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.