Closed Bug 934502 Opened 8 years ago Closed 7 years ago

Backtracking allocator: re-introduce spilling to argument slots


(Core :: JavaScript Engine: JIT, enhancement)

Not set





(Reporter: sunfish, Assigned: bhackett1024)




(5 files)

The backtracking allocator's code for spilling into argument slots was disabled for bug 906858, bug 931487, and bug 932465. However, it's a useful optimization, so it'd be nice to fix the problems and re-enable it.

The two main known problems are:
 - When there is an OSR entry, there can be two values defined by loads from the same argument slot --- one in each entry --- and these virtual registers are not always in the same group.
 - Argument slots are registered as GC roots as of bug 771398. On nunbox32 platforms, the register allocator may spill the two parts of a value separately, and if it spills only half of a value, the GC root may hold an invalid value.
Bug 936993 moved the MCheckOverRecursed instruction to avoid needing to have boxed and unboxed values of arguments live at the same time, which reduced the amount of spilling of argument values, which helps avoid this problem in some cases.

I'm also considering teaching the backtracking allocator to split arguments, which would eliminate most of the cases where there's a move from an argument slot directly to a spill slot. That may make the ability to spill into argument slots less important.
See Also: → 826741
Attached patch patchSplinter Review
This patch adds back argument spilling in the backtracking allocator and fixes the problems noted above:

- Parameter and OSR vregs have the same spill location so they are grouped together immediately, which will avoid issues with other vregs with overlapping lifetimes spilling to the arguments.

- The argument slots pushed by a caller can be marked by the caller when it is an Ion frame.  This is bug 771398, but the code associated with this bug is no longer necessary since argument slots are now pushed immediately before the call.

- The argument slots of an Ion frame are always marked when tracing that frame.  This logic is removed, so that safepoints are required to indicate how to trace the frame's arguments data.

For review, I'll split this patch into pieces which address each of the above.
Assignee: nobody → bhackett1024
Attached patch part 1Splinter Review
Group parameters with OSR entries.
Attachment #8551402 - Flags: review?(sunfish)
Attached patch part 2Splinter Review
Remove pushedArgumentSlots.
Attachment #8551404 - Flags: review?(jdemooij)
Attached patch part 3Splinter Review
Include arguments info in safepoints, rather than tracing them automatically.
Attachment #8551405 - Flags: review?(jdemooij)
Attached patch part 4Splinter Review
Restore argument spilling to the backtracking allocator.
Attachment #8551406 - Flags: review?(sunfish)
Comment on attachment 8551404 [details] [diff] [review]
part 2

Review of attachment 8551404 [details] [diff] [review]:

Nice, glad this works.
Attachment #8551404 - Flags: review?(jdemooij) → review+
Comment on attachment 8551405 [details] [diff] [review]
part 3

Review of attachment 8551405 [details] [diff] [review]:

::: js/src/jit/LIR.h
@@ +1240,5 @@
>      void rewriteRecoveredInput(LUse input);
>  };
> +struct SafepointSlotEntry {
> +    uint32_t stack:1;

Nit: add a one-line comment to explain this.

@@ +1247,5 @@
> +    SafepointSlotEntry() { }
> +    SafepointSlotEntry(bool stack, uint32_t slot)
> +      : stack(stack), slot(slot)
> +    { }
> +    SafepointSlotEntry(const LAllocation *a)

Nit: should be `explicit`.
Attachment #8551405 - Flags: review?(jdemooij) → review+
Comment on attachment 8551402 [details] [diff] [review]
part 1

Review of attachment 8551402 [details] [diff] [review]:

In the future, we may want to split these apart and allocate them separately instead of together as a group, but for now this looks fine.
Attachment #8551402 - Flags: review?(sunfish) → review+
Comment on attachment 8551406 [details] [diff] [review]
part 4

Review of attachment 8551406 [details] [diff] [review]:

Attachment #8551406 - Flags: review?(sunfish) → review+
Parts 2 and 3:

InvokeFunction() in VMFunctions.cpp was missing an auto rooter.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1128490
You need to log in before you can comment on or make changes to this bug.