557286 - Clean up mess around floating point layouts

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Lars T Hansen]

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.

Comment 1

[Lars T Hansen]

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

[Edwin Smith]

I beleive old ARM abi's also had this property.  arm-linux on IYONIX pc's, and strongarm/xscale generally, come to mind.

Comment 3

[Lars T Hansen]

Some details here: http://wiki.debian.org/ArmEabiPort#ARMfloatingpoints.  Emphasis on "old" though, looks like VFP cleaned this up.

Comment 4

[Edwin Smith]

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

[Rick Reitmaier]

Here's another bug (bug #549296) with some interesting comments about alignment on ARM.

Comment 6

[Lars T Hansen]

Attachment: Some initial sketches

Comment 7

[Lars T Hansen]

Attachment: Patch

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.

Comment 8

[Edwin Smith]

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+).

Comment 9

[Steven Johnson]

> PPC64 and x86-64 both support unaligned double loads, to the best of my knowlege

x86-64, yes, but PPC64? Really?

Comment 10

[Edwin Smith]

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"

Comment 11

[Lars T Hansen]

(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.

Comment 12

[Lars T Hansen]

tamarin-redux changeset:   4618:b20fa7279d02