604335 - asm_nongp_copy doesn't support move between GpRegs and FpRegs on SPARC

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Ginn Chen]

Found by lirasm random tests with debug version.

Comment 1

[Ginn Chen]

Attachment: patch

Not sure if we need the same for ARM.

Comment 2

[Edwin Smith]

Comment on attachment 483136 [details] [diff] [review]
patch

Seems harmless enough.  Does one of the existing lirasm test cases require this fix?  if not, could you add a new test case to ensure the fix is tested on sparc?

Comment 3

[Rick Reitmaier]

Comment on attachment 483136 [details] [diff] [review]
patch

Why is the #elif in there in the first place?   It should be #else 

BTW, we should be trying to eliminate any platform specific code from Assembler.  The i386 stuff in there is a regrettable artifact of supporting x87 fpu.

Comment 4

[Jacob Bramley [:jbramley]]

(In reply to comment #1)
> Not sure if we need the same for ARM.

I don't think it makes sense for ARM. We can't move a 64-bit float into a 32-bit integer register. Floating point and integer quantities are completely separated in almost all of the code.

On that note, I don't think it makes sense for MIPS either, but I don't know how their back-end does things so it might have some behaviour requiring that code.

Comment 5

[Jacob Bramley [:jbramley]]

Comment on attachment 483136 [details] [diff] [review]
patch

I don't know SPARC, but we don't want ARM on the list.

Comment 6

[Jacob Bramley [:jbramley]]

(In reply to comment #3)
> Why is the #elif in there in the first place?   It should be #else 

I disagree, as this code is not applicable to processors with 32-bit general-purpose registers. See comment #4.

Comment 7

[Rick Reitmaier]

(In reply to comment #6)
> (In reply to comment #3)
> > Why is the #elif in there in the first place?   It should be #else 
> 
> I disagree, as this code is not applicable to processors with 32-bit
> general-purpose registers. See comment #4.

Then why is this code here at all? 

The caller is expecting to move data from one register set to another.  If we want to support this 'optimization' then lets be more explicit about it.   A back-end method to query allowMovesAcrossRegisterSets() which then falls into the asm_nongp_copy() case.

For the #elif portion, we're expecting to do the move via spilling across the stack, which as you mentioned is only valid if the registers sets are of equal sized storage.  For which we can have an assert.

All of this is outside of the fix that Ginn should be providing, but if this sounds correct, I'll open a bug on it.

Comment 8

[Ginn Chen]

(In reply to comment #4)
> (In reply to comment #1)
> > Not sure if we need the same for ARM.
> 
> I don't think it makes sense for ARM. We can't move a 64-bit float into a
> 32-bit integer register. Floating point and integer quantities are completely
> separated in almost all of the code.

That's exact why we need the code and this patch.

I guess the title of this bug is misleading.
The patch is not going to implement moving between Gp and Fp for asm_nongp_copy.
The code is to avoid moving between registers if they're different types.

Comment 9

[Ginn Chen]

http://hg.mozilla.org/projects/nanojit-central/rev/004571303a08

Comment 10

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/aedbe1e95c92

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/aedbe1e95c92

Comment 12

[Rick Reitmaier]

http://hg.mozilla.org/tamarin-redux/rev/172dfb67c782