Add newlib cbrtf

[tesuji]

Ported from https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libm/common/sf_cbrt.c;hb=HEAD

Block on #173

Closes #134

[tesuji]

I honestly don't know why it failed. And I need some review!

[alexcrichton]

Thanks for the PR! I have the same questions as #169 for this though. Why is this change being made relative to what's currently already implemented?

[tesuji]

This implementation does not use double (f64) type. So it could run on
hardwares that have no support for double precision floats.

[alexcrichton]

Ok that makes sense to me, although does Rust run on any architectures that are like that? (are there examples today of platforms that would benefit?)

Review-wise there's no need to keep the musl code around, we only need one implementation of this function in-tree.

If the purpose here as well is to avoid f64, could some form of CI check be added to assert that f64 isn't used?

[Schultzer]

@lzutao can you please rebase your PR's and align them with #169 so we only have one impl. in the tree? We got a benchmark suite so we can evaluate each function now.

[tesuji]

I don't understand what you mean. This PR doesn't have any relations
with exp2f in #169 .

[tesuji]

Ok. I rebased on master and add your commit in #169.

[Schultzer]

@lzutao I'm sorry, what I meant was that we only want one implementation in the tree, we don't want to have a musl module or a newlib module. The benchmark suite is to give a rough estimate, ideally this should be tested on hardware.

[tesuji]

@Schultzer Do you know what wrong with this PR?
The ported cbrtf has the same generated asm as C version: https://godbolt.org/z/tfeV0c
as far as I understand.

[Schultzer]

The errors here are properly expected, I think the system on the ci uses musl and, as far as I know given the different implementation in newlib, we properly want to test this function against newlib or, increase the ULP for this case.

[gnzlbg]

If this passes an exhaustive check against musl (testing all f32 bit patterns), and the benchmarks say this is faster. I see no reasons against making this change.

[tesuji]

benchmarks say this is faster. I see no reasons against making this change.

I don't expect the ported newlib functions could be faster than old one. The reason
for newlib is to run on system have no hardware support for double precision floats.
See more in the issue #134 .

[gnzlbg]

@lzutao so will this be a performance regression on systems that do have support for double precision floats ?

[tesuji]

At least in x86_64, it doesn't have regression:
This PR:

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

Master:

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

[alexcrichton]

It looks like this doesn't change the performance, so is this still necessary?

[burrbull]

It should be tested on different platform, especially on embedded. With/without f32/f64 FPU.
Benchmark system should support different targets.

[tesuji]

Seems like we don't agree the merge this. Closing then.