The default bug view has changed. See this FAQ.

IonMonkey: remove spillRegs()

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
David suggested this in bug 709731.
There is a bug in the LSRA which show up in the implementation of Bug 713693. The bug is that bogus registers used to force spilling virtual registers are not working.

If they are working for now with current VMCalls is due to the spillRegs function which allocate extra-space (in spillRegs) for the return value, which is distributed to LSRA inputs.  In Bug 713693, the problem appeared because we have more inputs than outputs and inputs of LSRA cannot allocate a register since bogus registers have already allocated them.

Replacing the call to ins->spillReg() by RegisterSet::All() in LSRA code make the bug appear for the rest of the VMCalls.

In addition, spill regs should avoid to collect free registers with RegisterSet::take* functions.  The reason is that it will always return the same set of registers which may implies extra moves when the output of the previous instruction is fixed, such as in a sequence of call.
Blocks: 713693
(Assignee)

Comment 2

5 years ago
(In reply to Nicolas B. Pierron [:pierron] from comment #1)
> In Bug 713693, the problem appeared
> because we have more inputs than outputs and inputs of LSRA cannot allocate
> a register since bogus registers have already allocated them.

Yeah I think if you have *any* regular use (not at-start), you'll run into trouble atm. Sorry, I should have made this more clear by asserting on it or something. It's a bad restriction, but it's hard to fix properly, I'll see if I can get this to work like you'd expect.
Could we make use-at-start implicit for call uses?
(Assignee)

Comment 4

5 years ago
Created attachment 586087 [details] [diff] [review]
Part 1: Remove spillRegs

Slightly different way to handle spill intervals with LSRA. This seems less complicated and allows us to remove spillRegs(). The patch also moves some small methods to the header file -- 51 insertions(+), 100 deletions(-)
Attachment #586087 - Flags: review?(dvander)
(Assignee)

Comment 5

5 years ago
Created attachment 586141 [details] [diff] [review]
Part 2: Assert that call uses are at-start

(In hindsight, I could have added this in the previous patch, but I initially had other plans)

It's hard to support useRegister for calls, so requiring at-start seems the best approach. I'd rather not have calls implicitly use at-start though, to make it clear when reading the code that temps/defs could be assigned the same register.
Attachment #586141 - Flags: review?(dvander)
Attachment #586087 - Flags: review?(dvander) → review+
Attachment #586141 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/a89c5ef5da36

(merged both patches using qfold)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.