Skip to content
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

port newlib's hypotf #137

Open
japaric opened this issue Jul 27, 2018 · 8 comments · May be fixed by #240
Open

port newlib's hypotf #137

japaric opened this issue Jul 27, 2018 · 8 comments · May be fixed by #240
Labels
help wanted Extra attention is needed
Milestone

Comments

@japaric
Copy link
Member

japaric commented Jul 27, 2018

Source: https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libm/mathfp/ef_hypot.c;hb=HEAD

Rationale: the current implementation makes use of f64 and it has bad performance on architectures that have no hardware support for double precision floats.

What needs to be done:

  • Move the existing MUSL port into a private math::musl module and make sure we continue compiling it.
  • Port the newlib version into src/math/$fun.rs
  • Update the test suite by uncommenting the function name in tests/newlib.rs and removing the function name from tests/musl.rs.
@japaric japaric added the help wanted Extra attention is needed label Jul 27, 2018
@japaric japaric added this to the newlib milestone Jul 27, 2018
@burrbull
Copy link
Contributor

burrbull commented Aug 2, 2018

#141

@ChiefMilesEdgeworth
Copy link

I'd be interested in taking this up. I haven't contributed to a Rust repo before, so I'd need a bit of help with structure, but this shouldn't be too hard 🤞

@ChiefMilesEdgeworth
Copy link

For instance, there are two functions, GET_FLOAT_WORD and SET_FLOAT_WORD in the newlib implementation, and I don't know what the equivalent is for this library.

@Lokathor
Copy link
Contributor

Lokathor commented Apr 8, 2020

Looks like those are defined in fdlibm.h

They seem to be converting f32 to u32 and back via union type punning (don't compile that code in C++ mode!)

In Rust you'd use f32::to_bits and f32::from_bits instead.

@ChiefMilesEdgeworth
Copy link

from_bits gives back a u32, but the newlib code uses i32. I just want to make sure it's ok to use u32 instead, and if not, are the steps I need to take to guarantee that it is castable?

@Lokathor
Copy link
Contributor

Lokathor commented Apr 8, 2020

Converting between u32 and i32 with as will preserve the bit pattern (just not necessarily the numeric value), so feel free to use an extra as here and there if you're just trying to do a bit manipulation of a number.

@ChiefMilesEdgeworth
Copy link

So, the issue mentions moving the previous implementation to a private module. Could you point me to where I would put that? Is it in the same file, or some other part of the structure? Just an example file that has done this already would be fine, so that I can look at how they did it.

@Lokathor
Copy link
Contributor

Lokathor commented Apr 8, 2020

I don't see a location where a private module has been setup like that before, but I think I can describe the steps to do what @japaric was thinking:

  1. Make a musl folder in src/math/
  2. Move the current src/math/hypotf.rs file into that sub-folder
  3. Within src/math/mod.rs place the following code after the "private re-import" portion to make sure that we keep building the old version during tests:
// Test that the old code continues to build
#[cfg(test)]
#[allow(dead_code)]
mod musl {
    mod hypotf;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants