If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

Core Graveyard
Nanojit
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Ginn Chen, Assigned: Ginn Chen)

Tracking

Trunk
Sun
OpenSolaris

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey. fixed-in-tamarin)

Attachments

(1 attachment)

881 bytes, patch
Edwin Smith
: review+
jbramley
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Found by lirasm random tests with debug version.
(Assignee)

Comment 1

7 years ago
Created attachment 483136 [details] [diff] [review]
patch

Not sure if we need the same for ARM.
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #483136 - Flags: review?(edwsmith)

Comment 2

7 years ago
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?
Attachment #483136 - Flags: review?(edwsmith)
Attachment #483136 - Flags: review+
Attachment #483136 - Flags: feedback?(Jacob.Bramley)

Comment 3

7 years ago
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.
(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 on attachment 483136 [details] [diff] [review]
patch

I don't know SPARC, but we don't want ARM on the list.
Attachment #483136 - Flags: feedback?(Jacob.Bramley) → feedback+
(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

7 years ago
(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.
(Assignee)

Comment 8

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
http://hg.mozilla.org/projects/nanojit-central/rev/004571303a08
Whiteboard: fixed-in-nanojit
http://hg.mozilla.org/tracemonkey/rev/aedbe1e95c92
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey

Comment 11

7 years ago
http://hg.mozilla.org/mozilla-central/rev/aedbe1e95c92
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 12

7 years ago
http://hg.mozilla.org/tamarin-redux/rev/172dfb67c782
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey. fixed-in-tamarin
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.