Skip to content
This repository was archived by the owner on Mar 11, 2020. It is now read-only.

musl erf/erfc port #3

Merged
merged 1 commit into from
Aug 22, 2016
Merged

musl erf/erfc port #3

merged 1 commit into from
Aug 22, 2016

Conversation

musm
Copy link
Collaborator

@musm musm commented Aug 19, 2016

musl port

There is also the possibility of a sleef port and to benchmark the two implementations.

Edit:
These implementations are just as fast as Base.

@musm musm changed the title Initial erfc port, hacky but seems to work. Initial erf/erfc port, hacky but seems to work. Aug 19, 2016
hi = parse(UInt32,hi,2)
return hi
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done more efficiently by

function GET_HIGH_WORD(d::Float64)
    u = reinterpret(UInt64, d)
    (u >> 32) % UInt32
end

@simonbyrne
Copy link
Member

simonbyrne commented Aug 19, 2016

Aside from the notes by @alyst and myself, this actually looks rather reasonable. Try some profiling to see where the remaining bottlenecks are.

return hi
end

function SET_LOW_WORD(d::Float64,lo::UInt)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SET_LOW_WORD(d::Float64, lo::UInt32) = reinterpret(Float64, reinterpret(UInt64, d) & 0xFFFFFFFF00000000 | lo)

should be faster.

Also note that in general to get faster code you should avoid changing the type of the variable throughout the function as it makes Julia generating type-agnostic (much slower) code:

julia> @code_warntype SET_LOW_WORD(1.0, UInt(5))
Variables:
  #self#::#SET_LOW_WORD
  d@_2::Float64
  lo@_3::UInt64
  db::String
  lb::String
  d@_6::Any
  lo@_7::Any

Body:
  begin 
      lo@_7::Any = lo@_3::UInt64
      d@_6::Any = d@_2::Float64
      db::String = $(Expr(:invoke, LambdaInfo for bin(::UInt64, ::Int64, ::Bool), :(Base.bin), :((Base.box)(UInt64,d@_6::Float64)), 64, false)) # line 3:
      lo@_7::Any = (Base.box)(UInt32,(Base.checked_trunc_uint)(UInt32,lo@_7::UInt64)) # line 4:
      lb::String = $(Expr(:invoke, LambdaInfo for bin(::UInt32, ::Int64, ::Bool), :(Base.bin), :((Base.box)(UInt32,lo@_7::UInt32)), 32, false)) # line 5:
      SSAValue(0) = $(Expr(:invoke, LambdaInfo for getindex(::String, ::UnitRange{Int64}), :(Main.getindex), :(db), :($(Expr(:new, UnitRange{Int64}, 1, :((Base.select_value)((Base.sle_int)(1,32)::Bool,32,(Base.box)(Int64,(Base.sub_int)(1,1)))::Int64))))))
      d@_6::Any = $(Expr(:invoke, LambdaInfo for string(::String, ::String, ::Vararg{String,N}), :(Base.string), SSAValue(0), :(lb))) # line 6:
      d@_6::Any = $(Expr(:invoke, LambdaInfo for parse(::Type{Int64}, ::String, ::Int64), :(Main.parse), :(Main.Int), :(d@_6::String), 2)) # line 7:
      d@_6::Any = (Base.box)(Float64,d@_6::Int64) # line 8:
      return d@_6::Float64
  end::Float64

(::Any signifies type inference problem)

@musm musm changed the title Initial erf/erfc port, hacky but seems to work. Initial erf/erfc port Aug 19, 2016
@musm
Copy link
Collaborator Author

musm commented Aug 19, 2016

Thank you @simonbyrne & @alyst

# origin: FreeBSD /usr/src/lib/msun/src/s_erf.c

# * ====================================================
# * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should list this kind of thing in the top-level license file

@codecov-io
Copy link

codecov-io commented Aug 19, 2016

Current coverage is 100% (diff: 100%)

No coverage report found for master at e4c2c15.

Powered by Codecov. Last update e4c2c15...8c35271

@@ -20,3 +20,6 @@ The Libm.jl package is licensed under the MIT "Expat" License:
> OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> SOFTWARE.
>
Libm.jl includes code from the following projects, which have their own licenses:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a blank line before this or it'll be highlighted as if it's part of the quote

@musm
Copy link
Collaborator Author

musm commented Aug 19, 2016

what's going on with codecov, it looks like it has my wrong username on there?

@musm musm changed the title Initial erf/erfc port WIP erf/erfc port Aug 19, 2016
# Utils

# Get the more significant 32 bit int from a double
function GET_HIGH_WORD(d::Float64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we should probably try to stick to the convention of constants in uppercase, types in upper camel case, and functions and variables in lowercase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perhaps also a good candidate as a utility function to add in a different file.

@tkelman
Copy link

tkelman commented Aug 19, 2016

I wonder whether we shouldn't take a lesson-learned from openlibm and try to make this an automated translation from the upstream musl source code, if that would be possible? That way future updates to pull in bug fixes would be simpler and less dependent on when we happen to do individual ports.

@musm
Copy link
Collaborator Author

musm commented Aug 19, 2016

Perhaps, but most of the bug reports are not related to math functions, which are infrequent.

@simonbyrne
Copy link
Member

Do we go with this implementation what about sleef ?

I don't think it's a problem to have multiple implementations in here: indeed, I would encourage it. I suspect there will be some tradeoff between accuracy, performance and vectorisation between different implementations that would be useful to compare.

x = abs(x)
s = 1/(x*x)
if ix < 0x4006db6d # |x| < 1/.35 ~ 2.85714
R = ra0+s*(ra1+s*(ra2+s*(ra3+s*(ra4+s*(ra5+s*(ra6+s*ra7))))))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this code and Base.Math.@horner s ra0 ra1 ra2 ra3 ra4 ra5 ra6 ra7 generate the same @code_native, but @horner approach looks clearer. Yet maybe on other machines @horner would generate more efficient machine code.

@ararslan
Copy link
Member

Thinking more about this, would it make more sense to have error functions as part of SpecialFunctions.jl?

@musm musm changed the title WIP erf/erfc port musl erf/erfc port Aug 20, 2016
@@ -1,7 +1,7 @@
# Get the more significant 32 bit int from a double
function get_high_word(d::Float64)
u = reinterpret(UInt64, d)
(u >> 32) % UInt32
(u>>32)%UInt32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the spaces? IMO it's much more readable with them in there.

@ararslan
Copy link
Member

ararslan commented Aug 20, 2016

probably easiest to turn on appveyor

@simonbyrne Would you be willing to put this repo on your AppVeyor account?

@@ -1,5 +1,8 @@
module Libm
using Base.Math.@horner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be import, not using

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're not extending it, so actually using is better if you're asking for a specific function

@@ -0,0 +1,143 @@

# see (http://git.musl-libc.org/cgit/musl/tree/src/math/erf.c) for implementation details
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs a copyright notice here: i.e. based on musl code, copyright whoever is on that code. Also would be good to add which git commit you used to make it easy to check for updates.

@musm
Copy link
Collaborator Author

musm commented Aug 21, 2016

With the latest changes both erf and erfc are just as fast as Base!
I have to thank alyst and simonbyrne for their help

@tkelman
Copy link

tkelman commented Aug 21, 2016

needs a copyright notice here: i.e. based on musl code, copyright whoever is on that code. Also would be good to add which git commit you used to make it easy to check for updates.

And please clean your commit messages up.

@musm
Copy link
Collaborator Author

musm commented Aug 22, 2016

Does that look ok now?

@ViralBShah
Copy link
Member

For the first few functions, it would be great to see if a port from sleef gives us code that can benefit from SIMD. Would be good to compare and see how that goes.

+1 to @tkelman's comment about trying to do source-to-source wherever possible so that we can benefit from upstream as well. This is of course, good to have.

@ararslan ararslan mentioned this pull request Aug 22, 2016
@simonbyrne simonbyrne merged commit 4e37a25 into JuliaMath:master Aug 22, 2016
@simonbyrne
Copy link
Member

Great, thanks for doing this, and your patience with the reviews.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants