Closed Bug 714428 Opened 12 years ago Closed 12 years ago

IonMonkey: remove spillRegs()

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
(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?
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)
(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+
http://hg.mozilla.org/projects/ionmonkey/rev/a89c5ef5da36

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