Closed
Bug 557286
Opened 14 years ago
Closed 14 years ago
Clean up mess around floating point layouts
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P3)
Tamarin Graveyard
Virtual Machine
Tracking
(Not tracked)
RESOLVED
FIXED
Q3 11 - Serrano
People
(Reporter: lhansen, Assigned: lhansen)
Details
(Whiteboard: Has patch)
Attachments
(1 file, 1 obsolete file)
15.97 KB,
patch
|
edwsmith
:
review+
stejohns
:
review+
|
Details | Diff | Splinter Review |
Most code that needs to pick apart or put together floating point numbers just add an ad-hoc union of uint32_t[2] and double. However, this is not adequate for portability and we have AVMSYSTEM_DOUBLE_MSW_FIRST to deal with that (among other things). Most code does not pay attention to that setting (only some of the UNIX-specific mathutils code does). This needs to change.
Assignee | ||
Comment 1•14 years ago
|
||
In bug #555805 Steven says: "AFAIK, the only mainstream architecture with this property is MIPS (and even then maybe only some configs) -- as our MIPS support is still poorly tested, I'm not surprised if it's dodgy."
Comment 2•14 years ago
|
||
I beleive old ARM abi's also had this property. arm-linux on IYONIX pc's, and strongarm/xscale generally, come to mind.
Assignee | ||
Comment 3•14 years ago
|
||
Some details here: http://wiki.debian.org/ArmEabiPort#ARMfloatingpoints. Emphasis on "old" though, looks like VFP cleaned this up.
Comment 4•14 years ago
|
||
that matches my understanding. I almost wrote "old = pre-EABI" but wasn't sure if it was the introduction of VFP or EABI that cleaned it up. note that the windows arm abi is not the same as EABI, and VFP is not always supported, but I do beleive winmo uses 8-byte little endian for doubles, anyway.
Comment 5•14 years ago
|
||
Here's another bug (bug #549296) with some interesting comments about alignment on ARM.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → lhansen
Assignee | ||
Comment 6•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•14 years ago
|
||
I believe this patch addresses all relevant cases. It does not address some cases in nanojit where a double is mapped onto a uint64 or onto two uint32 and the order of those two does not matter (though I added a comment about naming in the latter case). It also does not address one chunk of code in AvmCore.cpp that applies to x86 and x86_64 only; the code may be worth fixing for the sake of consistency, opinions welcome (it would just be a renaming). Finally it does not address unions with float, where there are no interesting issues for us at present.
Attachment #438193 -
Attachment is obsolete: true
Attachment #443855 -
Flags: review?(stejohns)
Attachment #443855 -
Flags: review?(edwsmith)
Assignee | ||
Updated•14 years ago
|
Whiteboard: Has patch
Comment 8•14 years ago
|
||
Comment on attachment 443855 [details] [diff] [review] Patch nice cleanup, no obvious injections Interpreter.cpp, INSTR(push_doublebits): I'd expect it to be a few less instructions and cycles on a 64-bit cpu to simply do an unaligned load-double from the wordcode instruction stream. PPC64 and x86-64 both support unaligned double loads, to the best of my knowlege. maybe add a comment "fixme (bugXXX)" if out of scope for this bug. the comment in NativePPC.cpp is fine, but we assume everywhere than PPC is big-endian. not sure where the most obvious place for *that* comment should be. I can submit that in a separate patch to nanojit-central if you want. (or you can, consider it R+).
Updated•14 years ago
|
Attachment #443855 -
Flags: review?(edwsmith) → review+
Comment 9•14 years ago
|
||
> PPC64 and x86-64 both support unaligned double loads, to the best of my knowlege
x86-64, yes, but PPC64? Really?
Updated•14 years ago
|
Attachment #443855 -
Flags: review?(stejohns) → review+
Comment 10•14 years ago
|
||
consider this preliminary only #include <stdio.h> #include <stdint.h> int main(int argc, char **argv) { char data[100] = { 1, 2, 3, 4, 5, 6, 7, 8, 9 }; printf("data %p\n", data); int* intp = (int*)(data+1); short* shortp = (short*)(data+1); int64_t* int64p = (int64_t*)(data+1); double* doublep = (double*)(data+1); printf("unaligned int %d\n", *intp); printf("unaligned short %d\n", *shortp); printf("unaligned int64 %d\n", *int64p); printf("unaligned double %g\n", *doublep); return 0; } prints this on ppc-32 and ppc-64 on the G5 data 0x7fff5fbff920 unaligned int 33752069 unaligned short 515 unaligned int64 101124105 unaligned double 5.67893e-299 i haven't done anything exhaustive to check if there are OS-traps happening, etc, but it "seems to work"
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #8) > Interpreter.cpp, INSTR(push_doublebits): I'd expect it to be a few less > instructions and cycles on a 64-bit cpu to simply do an unaligned load-double > from the wordcode instruction stream. PPC64 and x86-64 both support unaligned > double loads, to the best of my knowlege. maybe add a comment "fixme (bugXXX)" > if out of scope for this bug. Will do that. > the comment in NativePPC.cpp is fine, but we assume everywhere than PPC is > big-endian. not sure where the most obvious place for *that* comment should > be. I can submit that in a separate patch to nanojit-central if you want. (or > you can, consider it R+). I will moderate the comment in NativePPC.cpp and let you deal with the more general issue, if you think it is necessary.
Assignee | ||
Comment 12•14 years ago
|
||
tamarin-redux changeset: 4618:b20fa7279d02
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Flags: flashplayer-bug+
You need to log in
before you can comment on or make changes to this bug.
Description
•