Closed Bug 1325358 Opened 3 years ago Closed 2 years ago

CacheIR: consider reusing stack slots

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jandem, Assigned: jschulte)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf-])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch v1.patch (obsolete) — Splinter Review
I took a stab at this.
Attachment #8852592 - Flags: feedback?(jdemooij)
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.
Attachment #8852592 - Flags: feedback?(jdemooij) → feedback+
Attached patch v2.patch (obsolete) — Splinter Review
Thanks for the quick feedback!
Assignee: nobody → j_schulte
Attachment #8852592 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8855074 - Flags: review?(jdemooij)
Comment on attachment 8855074 [details] [diff] [review]
v2.patch

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

Looks great, thanks! Do you have Try access?
Attachment #8855074 - Flags: review?(jdemooij) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20a0a19c13e1
Reuse stack-slots, that we couldn't pop, for spilling in CacheIR. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/20a0a19c13e1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.