Last Comment Bug 714428 - IonMonkey: remove spillRegs()
: IonMonkey: remove spillRegs()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
:
Mentors:
Depends on: 709731
Blocks: IonMonkey 713693
  Show dependency treegraph
 
Reported: 2011-12-31 01:54 PST by Jan de Mooij [:jandem]
Modified: 2012-01-06 02:36 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Remove spillRegs (12.37 KB, patch)
2012-01-05 08:03 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review
Part 2: Assert that call uses are at-start (1.27 KB, patch)
2012-01-05 10:52 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-12-31 01:54:06 PST
David suggested this in bug 709731.
Comment 1 Nicolas B. Pierron [:nbp] 2012-01-04 19:02:12 PST
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.
Comment 2 Jan de Mooij [:jandem] 2012-01-05 01:20:58 PST
(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.
Comment 3 David Anderson [:dvander] 2012-01-05 01:22:08 PST
Could we make use-at-start implicit for call uses?
Comment 4 Jan de Mooij [:jandem] 2012-01-05 08:03:15 PST
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(-)
Comment 5 Jan de Mooij [:jandem] 2012-01-05 10:52:31 PST
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.
Comment 6 Jan de Mooij [:jandem] 2012-01-06 02:36:08 PST
http://hg.mozilla.org/projects/ionmonkey/rev/a89c5ef5da36

(merged both patches using qfold)

Note You need to log in before you can comment on or make changes to this bug.