Last Comment Bug 717374 - IonMonkey: Don't spill all registers for OOL VM calls
: IonMonkey: Don't spill all registers for OOL VM calls
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on:
Blocks: IonSpeed
  Show dependency treegraph
 
Reported: 2012-01-11 13:07 PST by Jan de Mooij [:jandem]
Modified: 2012-01-19 17:50 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[1/2] Declare wrapper registers (10.38 KB, patch)
2012-01-13 12:07 PST, Nicolas B. Pierron [:nbp]
jdemooij: review+
Details | Diff | Review
[2/2] Shrink spilled register. (6.49 KB, patch)
2012-01-13 12:09 PST, Nicolas B. Pierron [:nbp]
jdemooij: review+
Details | Diff | Review

Description Jan de Mooij [:jandem] 2012-01-11 13:07:14 PST
Follow-up for bug 717254.

Currently we spill all registers when making an OOL VM calls, but we should only spill registers that are live and either:

1) volatile
2) used by the callVM/generateVMWrapper machinery

For 2), we could probably define a mask or register set and use it when we need a register in callVM etc.
Comment 1 Nicolas B. Pierron [:nbp] 2012-01-11 13:26:23 PST
(In reply to Jan de Mooij (:jandem) from comment #0)
> Follow-up for bug 717254.
> 
> Currently we spill all registers when making an OOL VM calls, but we should
> only spill registers that are live and either:
> 
> 1) volatile
> 2) used by the callVM/generateVMWrapper machinery

  3) which have to appear in the gc-map.

> For 2), we could probably define a mask or register set and use it when we
> need a register in callVM etc.
Comment 2 Nicolas B. Pierron [:nbp] 2012-01-11 15:51:48 PST
This will come in 3 steps:
1/ Control the registers used by all generateVMWrapper functions (avoid MoveResolver undefined behavior space)
2/ Dump a machine state in generateVMWrapper in case of bailout/invalidation.
3/ Reduce the number of registers clobbered and instrument the FrameRecovery to understand the spilling policy of OOL VM calls.
Comment 3 Nicolas B. Pierron [:nbp] 2012-01-12 16:55:28 PST
(In reply to Nicolas B. Pierron [:pierron] from comment #2)
> This will come in 3 steps:
> 1/ Control the registers used by all generateVMWrapper functions (avoid
> MoveResolver undefined behavior space)
> 2/ Dump a machine state in generateVMWrapper in case of bailout/invalidation.

This seems to be handle by both Handle exception and the invalidation.  Thus this step is already implemented because the all way of failures are already creating a machine state.

> 3/ Reduce the number of registers clobbered and instrument the FrameRecovery
> to understand the spilling policy of OOL VM calls.
Comment 4 Nicolas B. Pierron [:nbp] 2012-01-13 12:07:48 PST
Created attachment 588487 [details] [diff] [review]
[1/2] Declare wrapper registers
Comment 5 Nicolas B. Pierron [:nbp] 2012-01-13 12:09:22 PST
Created attachment 588488 [details] [diff] [review]
[2/2] Shrink spilled register.
Comment 6 Jan de Mooij [:jandem] 2012-01-18 00:18:43 PST
Comment on attachment 588487 [details] [diff] [review]
[1/2] Declare wrapper registers

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

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +500,5 @@
>      // Save the current stack pointer as the base for copying arguments.
>      Register argsBase = InvalidReg;
>      if (f.explicitArgs) {
> +        argsBase = r10;
> +        regs.take(r10);

Doesn't x86 need something similar? Or is it okay to take any register there because arguments are not passed in registers?
Comment 7 Jan de Mooij [:jandem] 2012-01-18 00:53:35 PST
Comment on attachment 588488 [details] [diff] [review]
[2/2] Shrink spilled register.

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

Looks good, this will be useful for testing the safepoint code - it's not used atm so it's easy to regress.

However, I'm a bit worried about bug 715111. We may have to push all live registers for the invalidation code. I'm not sure though, so please discuss this with Chris. r=me if he thinks this approach is okay.

::: js/src/ion/shared/CodeGenerator-shared-inl.h
@@ +126,5 @@
> +        RegisterSet(GeneralRegisterSet(Registers::WrapperMask),
> +                    FloatRegisterSet(FloatRegisters::WrapperMask));
> +    RegisterSet restRegs =
> +        RegisterSet::Intersect(RegisterSet::Intersect(wrapperRegs, liveRegs),
> +                               RegisterSet::Not(gcRegs));

Nit: can you move the statements to determine restRegs to its own static function, and call it in saveLive and restoreLive?
Comment 8 Nicolas B. Pierron [:nbp] 2012-01-18 10:47:26 PST
(In reply to Jan de Mooij (:jandem) from comment #6)
> Comment on attachment 588487 [details] [diff] [review]
> [1/2] Declare wrapper registers
> 
> Review of attachment 588487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/x64/Trampoline-x64.cpp
> @@ +500,5 @@
> >      // Save the current stack pointer as the base for copying arguments.
> >      Register argsBase = InvalidReg;
> >      if (f.explicitArgs) {
> > +        argsBase = r10;
> > +        regs.take(r10);
> 
> Doesn't x86 need something similar? Or is it okay to take any register there
> because arguments are not passed in registers?

On x86, it is ok to take any register, because no registers are used for arguments.  This means that whatever you pick for argsBase, it won't collide with the argument registers.(In reply to Jan de Mooij (:jandem) from comment #7)
> Comment on attachment 588488 [details] [diff] [review]
> [2/2] Shrink spilled register.
> 
> Review of attachment 588488 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, this will be useful for testing the safepoint code - it's not
> used atm so it's easy to regress.
> 
> However, I'm a bit worried about bug 715111. We may have to push all live
> registers for the invalidation code. I'm not sure though, so please discuss
> this with Chris. r=me if he thinks this approach is okay.

I discussed with Chris before this patch.  We have 2 key points in the current implementation.

1/ GC registers are all pushed after the locals.
2/ Invalidation is creating a machine state (dumps all registers), then bug 715111 should fill the machine state with previously saved registers.

> ::: js/src/ion/shared/CodeGenerator-shared-inl.h
> @@ +126,5 @@
> > +        RegisterSet(GeneralRegisterSet(Registers::WrapperMask),
> > +                    FloatRegisterSet(FloatRegisters::WrapperMask));
> > +    RegisterSet restRegs =
> > +        RegisterSet::Intersect(RegisterSet::Intersect(wrapperRegs, liveRegs),
> > +                               RegisterSet::Not(gcRegs));
> 
> Nit: can you move the statements to determine restRegs to its own static
> function, and call it in saveLive and restoreLive?

Good idea.

Note You need to log in before you can comment on or make changes to this bug.