934502 - Backtracking allocator: re-introduce spilling to argument slots

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

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.

Comment 1

[Dan Gohman [:sunfish]]

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.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: part 1

Group parameters with OSR entries.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: part 2

Remove pushedArgumentSlots.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: part 3

Include arguments info in safepoints, rather than tracing them automatically.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: part 4

Restore argument spilling to the backtracking allocator.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8551404 [details] [diff] [review]
part 2

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

Nice, glad this works.

Comment 8

[Jan de Mooij [:jandem]]

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

Comment 9

[Brian Hackett [Laid off!]]

Parts 2 and 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d56dfa4e845

Comment 10

[Dan Gohman [:sunfish]]

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.

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8551406 [details] [diff] [review]
part 4

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

Cool!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Brian Hackett (:bhackett) from comment #9)
> Parts 2 and 3:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6d56dfa4e845

Backed out for SM(ggc) permafail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92e6bed098d

https://treeherder.mozilla.org/logviewer.html#?job_id=5786852&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=5797196&repo=mozilla-inbound

Comment 13

[Brian Hackett [Laid off!]]

Parts 2 and 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/629c8aac3ece

InvokeFunction() in VMFunctions.cpp was missing an auto rooter.

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/629c8aac3ece

Comment 15

[Brian Hackett [Laid off!]]

Parts 1 and 4:

https://hg.mozilla.org/integration/mozilla-inbound/rev/61f7a945aea0

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61f7a945aea0