hardware independent isinf/isnan/isfinite #19008

musm · 2016-10-18T19:42:36Z

We can get rid of the custom Float16 handling of this and have all isinf be hardware independent.

Same thing with isnan ....

StefanKarpinski · 2016-10-18T19:57:25Z

base/float.jl

 isfinite(x::Integer) = true

-isinf(x::Real) = !isnan(x) & !isfinite(x)
+isinf(x::Real) = (reinterpret(Unsigned, x) & ~sign_mask(T)) == exponent_mask(T)


This is fine for specific floating-point formats (Float16, Float32, Float64) but not ok for generic Reals. I would leave the existing method for Real and add a more specific method for those types.

Good point, I'll do that.

Perhaps we also need an extra IEEEFloat abstract type, for which these things are valid?

@vchuravy has been using IntrinsicFloats
Either sound reasonable.

I like IEEEFloat, and will use it instead of my IntrinsicFloats.

musm · 2016-10-19T06:16:49Z

Sounds good for the IEEEFloat alias, updated PR with isinf, isnan, and isfinite

vchuravy

Do we have benchmarks for this?

vchuravy · 2016-10-19T07:48:09Z

base/float.jl

+isinf{T<:IEEEFloat}(x::T) = (reinterpret(Unsigned, x) & ~sign_mask(T)) == exponent_mask(T)
 isinf(x::Real) = !isnan(x) & !isfinite(x)

+


unnecessary newline

vchuravy · 2016-10-19T07:48:53Z

base/float.jl


 Test whether a floating point number is not a number (NaN).
 """
+isnan{T<:IEEEFloat}(x::T) = (reinterpret(Unsigned, x) & ~sign_mask(T)) > exponent_mask(T)


Cool did not know reinterpret(Unsigned, x) is a thing.

Is this more efficient than x != x?

It is not

# x != x julia> @code_native isnan(NaN) .text Filename: float.jl pushq %rbp movq %rsp, %rbp Source line: 355 ucomisd %xmm0, %xmm0 setp %al popq %rbp retq nopl (%rax) # This PR julia> @code_native isnan2(NaN) .text Filename: REPL[3] pushq %rbp movq %rsp, %rbp Source line: 1 movd %xmm0, %rax movabsq $9223372036854775807, %rcx # imm = 0x7FFFFFFFFFFFFFFF andq %rax, %rcx movabsq $9218868437227405312, %rax # imm = 0x7FF0000000000000 cmpq %rax, %rcx seta %al popq %rbp retq nopl (%rax,%rax)

I don't think we should sacrifice losing this hardware instructions when available.

See https://gist.github.com/musm/8150451b04120ef27974d8270dedbf6f
for more comprehensive tests (the gist is we are comparing performance in the 10-100 picosecond range...) So basically these instructions are so cheap and fast that it really doesn't matter on modern processors. In other words, the changes are just as fast as what we already have.

For isnan I agree, unfortunately we still require special handling for Float16 which prompted this pr, which I thought would be good to unify all the exceptional case handling to be independent of floating point hardware instructions so that Float16 (F128/F80) would be handled all the same. But the problem currently is that cpu support for small floats is not great (but I think now intel is investing more on this side due to demand).

I'll leave this to @vchuravy expertise on the issue on whether these proposed changes are positive or not, for now I am closing this.

For the denormals even though x - x will not work. It's only on the x87 chips where exceptional handling are very bad (some x87 sandybridge too?), where one may want to disable or flush them to zero.

But disabling denormals is not recommended, so the issue will probably never crop up; someone would have to go out of their way to disable them in Julia anyways.

So perhaps only for isinf and isfinite the changes are positive, but at that point might as well just use what we already have.

vchuravy · 2016-10-19T07:57:50Z

It seems we do have benchmarks https://github.com/JuliaCI/BaseBenchmarks.jl/blob/0e13bcd0e14337cfb6f6513a1d3d04b468b2abaa/src/scalar/ScalarBenchmarks.jl#L14, even though we should probably bench Float16 too

@nanosoldier runbenchmarks(ALL, vs=":master")

tkelman · 2016-10-19T10:41:55Z

base/float.jl

 isnan(x::Real) = false

+"""
+    isfinite(f) -> Bool


should this be removed from helpdb/Base.jl if it's also there and now duplicated?

simonbyrne · 2016-10-19T10:48:09Z

base/float.jl

+
+Test whether a floating point number is an infinity value (positive infinity or negative infinity).
+"""
+isinf{T<:IEEEFloat}(x::T) = (reinterpret(Unsigned, x) & ~sign_mask(T)) == exponent_mask(T)


Again, is this more efficient?

nanosoldier · 2016-10-19T11:03:22Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

musm · 2016-10-19T13:38:10Z

@vchuravy How meaningful is to benchmark changes that are within a fraction of a nanosecond?
The current base version may translate differently when compiled for another platform. x - x may fail for cpus with disabled subnormal support.

Apparently master branch of BenchmarkTools is now reliable to within ns range (but perhaps nanosoldier is not yet using the master branch).

See: https://gist.github.com/musm/8150451b04120ef27974d8270dedbf6f

vchuravy · 2016-10-19T13:47:22Z

I think Nanosoldier might still not use nanosecond resolution. Nevertheless changes to these kinds of function can have effects throughout the whole codebase
The regressions that nanosoldier is seeing are to big to be noise so it might be worthwhile seeing what the difference is.

nalimilan · 2016-10-19T13:57:28Z

The current base version may translate differently when compiled for another platform. x - x may fail for cpus with disabled subnormal support.

Would be good to have actual examples of such CPUs before making this method more complex.

musm · 2016-10-19T14:08:47Z

right, I highly doubt the second point...unless perhaps some ancient cpu that julia doesn't work on :)

jrevels · 2016-10-19T14:31:38Z

Just to clarify: BenchmarkTools/Nanosoldier has always had nanosecond resolution, so if that's what you require here, you've already got it. A recent PR has given BenchmarkTools picosecond resolution, but that hasn't been tagged/released yet because it breaks JLD (de)serialization compatibility with old data (i.e. all the data in BaseBenchmarkReports).

musm · 2016-10-19T17:30:20Z

@jrevels On the tagged version of BenchmarkTools the benchmark results are all stuck at 3ns it's only when I use the master version I obtain perceptible differences. It's likely that ns resolution is not enough here.

vchuravy · 2016-10-19T23:32:28Z

@musm Thank you for the effort you put into this. I will most likely do parts of this in #18927

StefanKarpinski requested changes Oct 18, 2016

View reviewed changes

musm changed the title ~~Is inf hardware indep~~ isinf hardware indep Oct 18, 2016

musm closed this Oct 18, 2016

musm force-pushed the isinf branch from b7b85ff to 1ea0de1 Compare October 18, 2016 20:01

musm reopened this Oct 18, 2016

hardware indep. isinf for hardware floats

0ceff46

musm force-pushed the isinf branch from c509ade to 0ceff46 Compare October 18, 2016 20:26

musm added 2 commits October 19, 2016 02:02

Add isnan functions

ed845a4

Add hard indep isfinite

f02dd4e

vchuravy approved these changes Oct 19, 2016

View reviewed changes

tkelman reviewed Oct 19, 2016

View reviewed changes

simonbyrne reviewed Oct 19, 2016

View reviewed changes

musm closed this Oct 19, 2016

musm changed the title ~~isinf hardware indep~~ hardware independent isinf/isnan/isfinite Oct 20, 2016

musm deleted the isinf branch April 24, 2017 19:03

		isinf{T<:IEEEFloat}(x::T) = (reinterpret(Unsigned, x) & ~sign_mask(T)) == exponent_mask(T)
		isinf(x::Real) = !isnan(x) & !isfinite(x)

Uh oh!

hardware independent isinf/isnan/isfinite #19008

hardware independent isinf/isnan/isfinite #19008

Uh oh!

Conversation

musm commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

musm Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

musm commented Oct 19, 2016

Uh oh!

vchuravy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p-zubieta Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

musm Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchuravy commented Oct 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nanosoldier commented Oct 19, 2016

Uh oh!

musm commented Oct 19, 2016

Uh oh!

vchuravy commented Oct 19, 2016

Uh oh!

nalimilan commented Oct 19, 2016

Uh oh!

musm commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrevels commented Oct 19, 2016

Uh oh!

musm commented Oct 19, 2016

Uh oh!

vchuravy commented Oct 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

musm commented Oct 18, 2016 •

edited

Loading

musm Oct 18, 2016 •

edited

Loading

p-zubieta Oct 19, 2016 •

edited

Loading

musm Oct 19, 2016 •

edited

Loading

musm commented Oct 19, 2016 •

edited

Loading