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

Add newlib exp2f #169

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Schultzer
Copy link
Contributor

@Schultzer Schultzer commented May 17, 2019

Hi guys,
I'm planning to deal we some of the "newlib issues" but I'm not sure in what direction we should go.

Alex mention in #162

we'd probably want to always pick some implementation to give to users and we could weigh newlib vs musl if we'd like?

Which I agree with.

Should we just annotate the function with #[cfg(source = "musl")] | #[cfg(source = "newlib")]?

fix #136

@alexcrichton
Copy link
Member

Looks like these two tests pass today but fail on ARM? Also I'm not entirely certain, but what's the purpose of replacing the function?

@Schultzer
Copy link
Contributor Author

Schultzer commented May 17, 2019

I think the rationale from japaric is that the musl version use f64 for some of the functions, which will yield slow performance on device that doesn't support f64.

The main goal for this PR is just to settle in which direction we should go.

Relating to the failing test, I don't really know, one thing too note is it's not related to #168 because it happens in both debug and release mode.

@alexcrichton
Copy link
Member

Ok, makes sense! In light of that would you be able to benchmark this and the previous implementation? I suspect the failing test is due to jn/jnf using this function transitively and its output is tweaked. They're probably not incorrect but the test harness at least needs some form of an update to avoid the failing tests.

@Schultzer
Copy link
Contributor Author

Schultzer commented May 17, 2019

I'm pretty sure that the falling test is not related to this PR. it happens on master too after #168.

I'm not entirely sure about the benchmark suite, can we assume that if we make a benchmark suite that runs in docker on CI, that it would have f64 native and emulation, depending on the target?

I don't have any embedding device to benchmark on, and would like to have it set up on CI, but if we cannot simulate real world, then I see little point in using it as a prof of performance, but I agree that we should have a benchmark suite, but I think it should be in a separate PR.

@burrbull
Copy link
Contributor

Some of ported newlib fns you can take here. https://github.com/burrbull/libm/tree/master/src/math/newlib

@Schultzer
Copy link
Contributor Author

Schultzer commented May 17, 2019

Thanks, it's great work, I had a branch going but I've ran into some troubles with the test generator because some of the functions had changed signature like pub(crate) fn rem_pio2f(x: f32) -> (i32, f64) -> pub fn rem_pio2f(x: f32) -> (i32, f32, f32)

That's why I started this PR to get going.

What do you think should we keep the musl or go for newlib?

On a side note do you have any idea why the tests are failing on ARM?

@alexcrichton
Copy link
Member

Thanks for opening #170 @Schultzer, it does indeed look unrelated! I think with a rebase and with another confirmation that CI is green this should be good to go, but I'm also still curious myself to learn more about moving to newlib and would prefer to have concrete data when changing existing implementations.

@Schultzer
Copy link
Contributor Author

I agree Alex, I would like to hold this PR back for a bit, until we have a benchmark suit.

@Schultzer
Copy link
Contributor Author

Schultzer commented Jul 2, 2019

Benchmark from master

test exp2f     ... bench:           5 ns/iter (+/- 0)

Benchmark for this pr

test exp2f     ... bench:           0 ns/iter (+/- 0)

@burrbull
Copy link
Contributor

burrbull commented Jul 2, 2019

Thanks for opening #170 @Schultzer, it does indeed look unrelated! I think with a rebase and with another confirmation that CI is green this should be good to go, but I'm also still curious myself to learn more about moving to newlib and would prefer to have concrete data when changing existing implementations.

When this repository was created, the main argument in favor of the musl was the ease of porting.

@Schultzer
Copy link
Contributor Author

@alexcrichton I know we are benchmarking on CI but, what do you think?

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 11, 2019

Are you calling the function with the same inputs both times? Otherwise the comparison isn't very meaningful.

@Schultzer
Copy link
Contributor Author

Could we benchmark against all f32, using the same value for binary and trinary functions?

@alexcrichton
Copy link
Member

FWIW a benchmark of 0ns/iter means that it's probably a bad benchmark because it's being optimized out. While I agree that this is simpler I still think that more investigation is needed into the performance to see whether it's worth it

@kpp
Copy link
Contributor

kpp commented Aug 10, 2019

Hey guys!
Is this PR is blocked because of lack of good benches? I can help.

@alexcrichton
Copy link
Member

Yes this and other PRs are blocked on evaluating how to proceed, and the best metric for now is speed and there hasn't been much benchmarking yet to see how to proceed.

@kpp kpp mentioned this pull request Aug 12, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

port newlib's exp2f
5 participants