429673 - ARM: Math failures running with THUMB or ARM jit and interp.

Status: VERIFIED FIXED

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

Description

[Brent Baker]

The are a handful of math failures when running with either the ARM or THUMB jit.

ecma3/Math/e15_8_2_17_1.as
Math.sqrt(3) = 1.7320508075688774 FAILED! expected: 1.7320508075688772
Math.sqrt(3) = 1.73205080756887742 FAILED! expected: 1.73205080756887719
Math.sqrt(3) = 1.732050807568877415 FAILED! expected: 1.732050807568877193
Math.sqrt(3) = 1.7320508075688774152 FAILED! expected: 1.7320508075688771932

ecma3/Math/e15_8_2_5.as
Math.atan2(0, -0) = 0 FAILED! expected: 3.141592653589793
Infinity/Math.atan2(-0, 1) = Infinity FAILED! expected: -Infinity
Math.atan2(-0,       -0) = 0 FAILED! expected: -3.141592653589793
Math.atan2(-0,       -1) = 3.141592653589793 FAILED! expected: -3.141592653589793
Math.atan2(Infinity, Infinity) = NaN FAILED! expected: 0.7853981633974483
Math.atan2(Infinity, -Infinity) = NaN FAILED! expected: 2.356194490192345
Math.atan2(-Infinity, Infinity) = NaN FAILED! expected: -0.7853981633974483
Math.atan2(-Infinity, -Infinity) = NaN FAILED! expected: -2.356194490192345

Comment 1

[Rob Borcic]

The ARM versions of atan2 don't differentiate between +0 and -0. We'd need to add some code to check for these conditions before calling atan2. e.g.:

	bool xnan = (((int*)(&x))[1] & 0x7FF80000) == 0x7FF80000;
	bool ynan = (((int*)(&y))[1] & 0x7FF80000) == 0x7FF80000;
		
	if (xnan || ynan)
	{
		return nan();
	}

	int sx = (((int*)(&x))[1] & 0x80000000);
	int sy = (((int*)(&y))[1] & 0x80000000);
		
	if (y == 0.0)
	{
		if (sy) // y is -0
		{
			if (sx) // x is <0 or -0
			{
				return -KPi;
			}
			else // x is >0 or +0
			{
				return -0.0;
			}
		}
		else // y is +0
		{
			if (sx) // x is <0 or -0
			{
				return KPi;
			}
			else // x is >0 or +0
			{
				return +0.0;
			}
		}
	}
	return ::atan2(y, x);

Comment 2

[Lars T Hansen]

Brent/Rob, this is marked as pertaining to TT; is that correct?  Even in TR/TC the testconfig.txt file has special handling of these tests.  Should I change the component?

Comment 3

[Brent Baker]

This now applies to TC/TR as there is an arm version which came from TT. I have updated the component to "Virtual Machine".

Comment 4

[Rick Reitmaier]

seeing these failures...reassign to me since i'm taking on the other arm issues; unless Lars has a ready fix.

BTW, I'd like to rename platform/arm/MathUtilsUnix to platform/arm/MathUtilsARM, but will check out the VMPI_ changes first.

Comment 5

[Rick Reitmaier]

Attachment: ver 1 - atan2 matches ecma spec for winmo

This is a winmo only fix.  If other platforms require this change then we need to log a new bug or annotate this one.  

Also, this does not modify the sqrt failures, which according to ecma docs are allowable.  

"Returns an implementation-dependent approximation to the square root of x."

We need to attach another patch to address this issue with the test suite.

Comment 6

[Lars T Hansen]

(In reply to comment #5)
> Created an attachment (id=366704) [details]
>
> This is a winmo only fix.  If other platforms require this change then we need
> to log a new bug or annotate this one.  

I will do so, because the mathutils stuff looks like it could probably do with a little cleaning up, at a minimum.  (For example, X86_MATH doesn't mean that at all, it means IA32 && Visual Studio; for another example, everyone not using that pays for a double function call to get to the math functions, but that shouldn't be necessary.  But this bug shouldn't have to fix that.)

Comment 7

[Rob Winchell]

Comment on attachment 366704 [details] [diff] [review]
ver 1 - atan2 matches ecma spec for winmo


>+    const static double PI = 3.141592653589793;
>+    const static double PI3_BY_4 = 3*PI/4;
>+    const static double PI_BY_4 = PI/4;

Any reason not to define PI3_BY_4 and PI_BY_4 as constant values? Avoid the floating point divide (expensive).

Comment 8

[Rick Reitmaier]

We could, I was concerned that the compiler might produce a different result than the runtime, but this is probably a bit of paranoia on my part.  I'll move them.

Comment 9

[Steven Johnson]

Yow, can't asc do this optimization at compile-time? I would presume/hope that the language would allow for this...

Comment 10

[Lars T Hansen]

More than that, you're looking at C++ code here and -O should do the trick :-)

Comment 11

[Steven Johnson]

doh! that was a doumb mistake on my part :-/

Comment 12

[Rob Borcic]

Comment on attachment 366704 [details] [diff] [review]
ver 1 - atan2 matches ecma spec for winmo

Your patch would handle the condition I found on Symbian using ARM RVCT 2.2.

Comment 13

[Rick Reitmaier]

pushed as 085ae23e9c68: bugfix #429673 - special case for winmo atan2 support (r+=robb) - rev 1590

btw, sqrt() issues will be addressed in test suite, since ecma spec states that result is dependent on implementation.

Comment 14

[Brent Baker]

Removed the expected fail for ecma3/Math/e15_8_2_5.as, pushed as 1596:74e23e3686ef

Rick what should be done for the sqrt() "failures", is the current result from windows mobile an acceptable value?

Comment 15

[Rick Reitmaier]

Yes it is.  I sent an earlier email regarding this issue. Any way the test suite can be tweaked to ignore the round-off difference?

Comment 16

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.