517939 - x64 regalloc_binary encounters LIR_u2f and LIR_float in GPRs as arguments, should we permit this?

Status: VERIFIED DUPLICATE of bug 521691

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Rick Reitmaier]

The change http://hg.mozilla.org/tamarin-redux/rev/089d4ff557a7 introduced code to ensure that both arguments to regalloc_binary are in correct registers.

The change pulled directly from Tracemonkey resulted in asserting on line 1.24.  So LIR_u2f and LIR_float were added to the original code and this was checked-into tamarin.

The bug is open for discussion on how we wish this resolve the issue.

Comment 1

[Rick Reitmaier]

It sounds like asm_store64 should use FpRegs if it sees either of those
instructions.

The comment is confusing because I forgot to end the first line with a
period. Sorry about that.

-dvander

On 9/18/09 1:31 PM, Rick Reitmaier wrote:
> In regalloc_binary() on NativeX64 as change was made as
> a result of bug 516093.
>
> Unfortunately this breaks tamarins' acceptance suite for
> mac x64 support.
>
> Specifically :
>
>          } else if (!(allow&  rmask(ra))) {
>              // rA already has a register assigned, but it's not valid
>              // to make sure floating point operations stay in FPU registers
>              // as much as possible, make sure that only a few opcodes are
>              // reserving GPRs.
>              NanoAssert(a->opcode() == LIR_quad || a->opcode() == LIR_ldq);
>              allow&= ~rmask(rr);
>              ra = findRegFor(a, allow);
>          }
>
> asserts on a->opcode().
>
> If I extend the assert to include LIR_u2f and LIR_float the
> regressions pass, but this doesn't seem like a proper fix.
>
> The underlying issue is that an stqi leads to asm_store64()
> which assigns a GPR to the instruction.
>
> If this instruction is used a parameter in regalloc_binary() we assert.
>
> Thoughts?
>
> - Rick

Comment 2

[Nicholas Nethercote [inactive]]

I hit this with the lirasm --random mode.  Changing asm_store64() as suggested by comment #1 is the short-term fix.

The proper fix is to distinguish float loads/stores from quad load/stores.

(Nb: bug 520208 is the TM version of this bug.)

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> 
> The proper fix is to distinguish float loads/stores from quad load/stores.

See bug 520714.

Comment 4

[Nicholas Nethercote [inactive]]

Bug 521691's patch fixes the symptom.  I'll mark this as a dup of that bug.