1325358 - CacheIR: consider reusing stack slots

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Jan de Mooij [:jandem]]

The IC register allocator sometimes has to spill values to the stack. If we know a stack slot is no longer needed (for instance, because we moved the value into a register and were unable to pop it), it would be easy to add it to a free list and reuse the slot if we have to spill something else.

Comment 1

[Johannes Schulte [:jschulte]]

Attachment: v1.patch

I took a stab at this.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8852592 [details] [diff] [review]
v1.patch

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

Very nice, thanks for taking this on! Please make sure to test this on x86 (32-bit) too, it uses the stack more than x64 because there are not as many registers.

::: js/src/jit/CacheIRCompiler.cpp
@@ +566,5 @@
>      if (loc->kind() == OperandLocation::ValueReg) {
> +        if (!freeValueSlots_.empty()) {
> +            uint32_t stackPos = freeValueSlots_.popCopy();
> +            masm.storeValue(loc->valueReg(), Address(masm.getStackPointer(),
> +                                                     stackPushed_ - stackPos));

We should assert stackPos <= stackPushed_ here and below.

@@ +632,5 @@
>          stackPushed_ -= sizeof(uintptr_t);
>      } else {
>          MOZ_ASSERT(loc->payloadStack() < stackPushed_);
>          masm.loadPtr(Address(masm.getStackPointer(), stackPushed_ - loc->payloadStack()), dest);
> +        masm.propagateOOM(freePayloadSlots_.append(loc->payloadStack()));

I think we could do something similar in freeDeadOperandRegisters. Would you mind trying that? We should then rename it to freeDeadOperandLocations or something.

@@ -626,5 @@
>  CacheRegisterAllocator::popValue(MacroAssembler& masm, OperandLocation* loc, ValueOperand dest)
>  {
>      MOZ_ASSERT(loc >= operandLocations_.begin() && loc < operandLocations_.end());
>      MOZ_ASSERT(stackPushed_ >= sizeof(js::Value));
> -

Nit: don't remove this line.

Comment 3

[Johannes Schulte [:jschulte]]

Attachment: v2.patch

Thanks for the quick feedback!

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8855074 [details] [diff] [review]
v2.patch

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

Looks great, thanks! Do you have Try access?

Comment 5

[Johannes Schulte [:jschulte]]

Attachment: v3.patch

Yup.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0722301e002cb8592b4653b316e8d399291c89d1

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20a0a19c13e1
Reuse stack-slots, that we couldn't pop, for spilling in CacheIR. r=jandem

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/20a0a19c13e1