Last Comment Bug 680432 - IonMonkey: Greedy Allocator: erasure of xmm register.
: IonMonkey: Greedy Allocator: erasure of xmm register.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 677337
  Show dependency treegraph
 
Reported: 2011-08-19 07:33 PDT by Nicolas B. Pierron [:nbp]
Modified: 2011-11-04 13:58 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case - Force usage of stack by adding pressure on registers. (1020 bytes, application/x-javascript)
2011-08-19 07:33 PDT, Nicolas B. Pierron [:nbp]
no flags Details
wip v0 (4.65 KB, patch)
2011-10-25 16:47 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
fix (43.19 KB, patch)
2011-10-27 11:06 PDT, David Anderson [:dvander]
sstangl: review+
Details | Diff | Splinter Review
follow-up fix (4.25 KB, patch)
2011-10-27 14:26 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
better follow-up fix (4.42 KB, patch)
2011-10-27 16:40 PDT, David Anderson [:dvander]
sstangl: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-08-19 07:33:12 PDT
Created attachment 554411 [details]
Test case - Force usage of stack by adding pressure on registers.

The attached test highlight the issue.  At the end, the first block has the following set of instructions:

    begin_LIR
      1 parameter ([x (arg:0)]) <|@
      2 parameter ([x (arg:8)]) <|@
      3 double ([f (=xmm14)]) <|@
      0 movegroup ()[=xmm14 -> stack:d5] <|@
      4 double ([f (=xmm13)]) <|@
      5 double ([f (=xmm11)]) <|@
      6 double ([f (=xmm10)]) <|@
      7 double ([f (=xmm9)]) <|@
      8 double ([f (=xmm8)]) <|@
      9 double ([f (=xmm7)]) <|@
      10 double ([f (=xmm6)]) <|@
      11 double ([f (=xmm5)]) <|@
      12 double ([f (=xmm4)]) <|@
      13 double ([f (=xmm3)]) <|@
      14 double ([f (=xmm2)]) <|@
      15 double ([f (=xmm1)]) <|@
      16 double ([f (=xmm0)]) <|@
      17 double ([f (=xmm12)]) <|@
      18 double ([f (=xmm14)]) <|@
      0 movegroup ()[=xmm14 -> stack:d3] <|@
*     19 double ([f (=xmm14)]) <|@
*     0 movegroup ()[stack:d5 -> =xmm14] <|@
      20 value ([x (=r15)]) <|@
      0 movegroup ()
         [=xmm14 -> stack:d4],
         [=xmm13 -> stack:d2],
         [=xmm11 -> stack:d1],
         [=xmm10 -> stack:d15],
         [=xmm9 -> stack:d14],
         [=xmm8 -> stack:d13],
         [=xmm7 -> stack:d12],
         [=xmm6 -> stack:d11],
         [=xmm5 -> stack:d10],
         [=xmm4 -> stack:d9],
         [=xmm3 -> stack:d8],
         [=xmm2 -> stack:d7],
         [=xmm1 -> stack:d6],
         [=xmm0 -> stack:d5],
         [=xmm12 -> stack:d3],
         [stack:d3 -> =xmm13] <|@
      21 goto () <|@
    end_LIR
Comment 1 David Anderson [:dvander] 2011-10-24 19:03:02 PDT
Interestingly, this fails on both allocators.
Comment 2 David Anderson [:dvander] 2011-10-25 16:47:25 PDT
Created attachment 569545 [details] [diff] [review]
wip v0

At the backedge of loops we store some information in the LDefinition::output field of phis. But this field is used to compute whether an instruction has canonical backing stack. So that's the first bug. But with this, the Greedy allocator still produces the wrong result - just now it's a normal number instead of NaN.
Comment 3 David Anderson [:dvander] 2011-10-27 11:06:03 PDT
Created attachment 570036 [details] [diff] [review]
fix

One more bug. The MoveEmitter was incorrect when a cycle involved memory-to-memory doubles. It was detecting the size via the MoveOperand, when it actually has to use the MoveKind.

While fixing this, I made MoveEmitter use ScratchFloatReg instead. This got rid of a ton of spill logic.
Comment 4 David Anderson [:dvander] 2011-10-27 14:26:02 PDT
Created attachment 570090 [details] [diff] [review]
follow-up fix

Heh. A third bug causing a failure in the same testcase, with GVN or LICM off. The problem is when instruction X has spill slot N. N gets freed before allocating registers, so it can be re-used as a spill slot during eviction. This messes up the spill/restore code. So I just moved stack slot killing later in the algorithm.
Comment 5 David Anderson [:dvander] 2011-10-27 16:40:59 PDT
Created attachment 570117 [details] [diff] [review]
better follow-up fix

This fix botched on redefines. Here's the right fix.

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