IonMonkey: Greedy Allocator: erasure of xmm register.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nbp, Assigned: dvander)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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
Interestingly, this fails on both allocators.
Blocks: 677337
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.
Assignee: general → dvander
Status: NEW → ASSIGNED
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.
Attachment #569545 - Attachment is obsolete: true
Attachment #570036 - Flags: review?
Attachment #570036 - Flags: review? → review?(sstangl)
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.
Attachment #570090 - Flags: review?(sstangl)
Created attachment 570117 [details] [diff] [review]
better follow-up fix

This fix botched on redefines. Here's the right fix.
Attachment #570090 - Attachment is obsolete: true
Attachment #570090 - Flags: review?(sstangl)
Attachment #570117 - Flags: review?(sstangl)

Updated

6 years ago
Attachment #570036 - Flags: review?(sstangl) → review+

Updated

6 years ago
Attachment #570117 - Flags: review?(sstangl) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/e6931fc5b630
http://hg.mozilla.org/projects/ionmonkey/rev/5b43306f9b17
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.