New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues handling floating point numbers #356

Merged
merged 1 commit into from Oct 27, 2016

Conversation

Projects
None yet
2 participants
@soc
Copy link
Contributor

soc commented Oct 26, 2016

There are two issues at play here, discovered while implementing
ArrayList#contains:

  • Boxing elimination replaces +0.0 values with -0.0 values that were
    defined ealier in the code (or the other way around).
    This happens because the the F32/F64 case classes synthesize the
    standard equals implementation which uses == and considers +0.0/-0.0
    equal (and identical NaNs values never equal).
    This is fixed by providing an equals method that compares the raw bits
    instead, making sure that we only deduplicate values that have the
    same bits.
  • The ArrayList#contains method was still failing as Float#equals and
    Double#equals were using ==. This was fixed by comparing the processed
    bits (all NaN bit patterns are collapsed into one).

The test suite is extended with tests for both zero and NaN.

@soc soc force-pushed the soc:topic/fp branch 2 times, most recently from 8e244c7 to ad7ed79 Oct 26, 2016

@densh
Copy link
Member

densh left a comment

Some minor change suggestions below.

theseBits == thoseBits
} else
false
}

This comment has been minimized.

@densh

densh Oct 26, 2016

Member

Those two equals implementations above would probably look better if you pattern match on that.

This comment has been minimized.

@soc

soc Oct 26, 2016

Author Contributor

I tried avoiding pattern matching, because this method is likely to be hot, and I wanted to make sure we don't worsen performance by some pattern matching artifacts.

Shall I change it?

This comment has been minimized.

@densh

densh Oct 26, 2016

Member

It should not make much difference. Lets change it.

This comment has been minimized.

@soc

soc Oct 27, 2016

Author Contributor

Alright, will do!

val x = Double.NaN
val y = longBitsToDouble(doubleToRawLongBits(x) | 1)
assert(x equals y)
}

This comment has been minimized.

@densh

densh Oct 26, 2016

Member

A few more tests cases would be nice to have here and for FloatSuite too.

This comment has been minimized.

@soc

soc Oct 26, 2016

Author Contributor

Any specific wishes?

This comment has been minimized.

@densh

densh Oct 26, 2016

Member

Some interesting values are: +0.0, 0.0, NaN, MaxValue, MinValue, -42, +42. Would be nice to add more asserts to check that those are equivalent when boxed independently. And not equivalent when you compare the different ones.

This comment has been minimized.

@soc

soc Oct 27, 2016

Author Contributor

Done!

// use doubleToLongBits instead of doubleToRawLongBits
val a = Double.doubleToLongBits(doubleValue)
val b = Double.doubleToLongBits(that.doubleValue)
a == b

This comment has been minimized.

@densh

densh Oct 26, 2016

Member

Harmony does nearly the same thing here. That's good that we converge on the same solution independently.

Fix issues handling floating point numbers
There are two issues at play here, discovered while implementing
ArrayList#contains:

- Boxing elimination replaces +0.0 values with -0.0 values that were
  defined ealier in the code (or the other way around).
  This happens because the the F32/F64 case classes synthesize the
  standard equals implementation which uses == and considers +0.0/-0.0
  equal (and identical NaNs values never equal).
  This is fixed by providing an equals method that compares the raw bits
  instead, making sure that we only deduplicate values that have the
  same bits.

- The ArrayList#contains method was still failing as Float#equals and
  Double#equals were using ==. This was fixed by comparing the processed
  bits (all NaN bit patterns are collapsed into one).

The test suite is extended with tests for both zero and NaN.

@soc soc force-pushed the soc:topic/fp branch from ad7ed79 to 6733f69 Oct 27, 2016

// assertNot(x == y) // FIXME this works on the JVM, but fails on native.

val z = longBitsToDouble(doubleToLongBits(x) | 1)
// assertNot(x == z) // FIXME this works on the JVM, but fails on native.

This comment has been minimized.

@soc

soc Oct 27, 2016

Author Contributor

If this is ok for you, I'd like to defer investigation of this to another issue/PR to be able to create PRs for the work I have already done that depends on the fixes in this PR.

I haven't figured out whether this is actually a real bug yet (the OpenJDK explicitly warns that the NaN bit patterns might change and are platform-dependent) or a valid variation.

This comment has been minimized.

@soc

soc Oct 27, 2016

Author Contributor

One reason why this might happen is that we use fcmp ueq which returns true if either operand is unordered (NaN). Maybe fcmp oeq captures the semantics of the JVM's == for floating point numbers more precisely?

This comment has been minimized.

@densh

densh Oct 27, 2016

Member

Seems like unrelated issue, let's defer it for later PR.

This comment has been minimized.

@densh
@densh

This comment has been minimized.

Copy link
Member

densh commented Oct 27, 2016

LGTM, thanks @soc !

@densh densh merged commit fb52893 into scala-native:master Oct 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment