717374 - IonMonkey: Don't spill all registers for OOL VM calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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]]

(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]]

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]]

(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]]

Attachment: [1/2] Declare wrapper registers

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: [2/2] Shrink spilled register.

Comment 6

[Jan de Mooij [:jandem]]

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]]

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]]

(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.

Comment 9

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/fc8fd0998c1b
https://hg.mozilla.org/projects/ionmonkey/rev/d3f45b54aeb3