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

[WIP] Implement cbrt and hypot function calls #763

Merged
merged 1 commit into from
Jun 12, 2019

Conversation

kungfukennyg
Copy link
Contributor

Implements missing cbrt/hypot function calls as per #667.

Note that this PR is in progress -- I still need to determine what other function calls are missing.

@RalfJung
Copy link
Member

RalfJung commented Jun 6, 2019

Closing and re-opening to hopefully deconfuse Travis.

@RalfJung RalfJung closed this Jun 6, 2019
@RalfJung RalfJung reopened this Jun 6, 2019
@RalfJung
Copy link
Member

RalfJung commented Jun 6, 2019

Hm dang. Seems like Travis just does not test "Draft" PRs, but AppVeyor does? This is silly.

@RalfJung RalfJung changed the title Implement cbrt and hypot function calls [WIP] Implement cbrt and hypot function calls Jun 6, 2019
@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jun 6, 2019
@kungfukennyg kungfukennyg marked this pull request as ready for review June 6, 2019 16:23
@kungfukennyg
Copy link
Contributor Author

I marked it as ready for review, let's see if that helps.

@kungfukennyg kungfukennyg force-pushed the fix_cmath_functions branch from b2b4261 to 95912a0 Compare June 6, 2019 18:27
@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2019

It does, and shows a legit failure:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1.6261333316791688`,
 right: `1.6261333316791686`', $DIR/cmath.rs:10:5

That's why I don't like floating point stuff. ;)

}

fn main() {
assert_eq!(cbrt(4.3), 1.6261333316791686);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you can round both values or use one of the "almost eq" crates for float assertions

Copy link
Member

Choose a reason for hiding this comment

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

Well, not really the crates, but inlining that shouldn't be too hard, even without fabs.

fn almost_eq(x: f64, y: f64) -> bool {
    const EPSILON: f64 = 1e-20;
    let diff = x-y;
    diff < EPSILON && diff > -EPSILON
}

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2019

Also, this "non-determinism" (not really but almost) makes me wonder if we should require a flag? This will mean that e.g. a "cross-Miri" running Linux code on macOS will not behave the same than when "native Miri" runs Linux code on Linux.

But I also don't want 200 flags. So I am inclined to just accept this kind of divergence here. Once we have a "communication with the host OS" flag, we could use that.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 7, 2019

Well... the clean solution would be to add these functions to librustc_apfloat so we have actual soft impls that are the same everywhere

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2019

Sure, that would be best.

@kungfukennyg
Copy link
Contributor Author

Would I add the functions to librustc_apfloat/lib.rs?

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2019

I don't know but @eddyb would. :)

@oli-obk
Copy link
Contributor

oli-obk commented Jun 7, 2019

oh, eh, implementing those may be very hard. I have no clue what's needed to implement them properly. There may be standards to read for edge cases and stuff.

src/fn_call.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Jun 8, 2019

FWIW, this is actually not the first Miri "intrinsic" that uses host floats; we have sin/cos/pow/... already implemented and they all use host floats right now (for lack of a softfloat implementation).

@oli-obk
Copy link
Contributor

oli-obk commented Jun 9, 2019

Well... the clean solution would be to add these functions to librustc_apfloat so we have actual soft impls that are the same everywhere

Wasn't meant to block this PR. My original suggestion to just round in the test still stands.

@RalfJung
Copy link
Member

RalfJung commented Jun 10, 2019

Actually we even already have the macro for that in tests/run-pass/intrinsics-math.rs.

@kungfukennyg kungfukennyg force-pushed the fix_cmath_functions branch from 95912a0 to a8f66ef Compare June 11, 2019 00:17
@kungfukennyg
Copy link
Contributor Author

kungfukennyg commented Jun 11, 2019

Okay I moved my test cases to the pre-existing intrinsics-math.rs and modified to use the assert_approx_eq macro.

Edit: Looks like the windows builds aren't happy.

src/fn_call.rs Outdated
let n = f.cbrt();
this.write_scalar(Scalar::from_f64(n), dest)?;
}
"hypot" => {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is called _hypot on Windows, so maybe just add that as an alternative case here?

@kungfukennyg kungfukennyg force-pushed the fix_cmath_functions branch from a8f66ef to 0a42d0b Compare June 11, 2019 16:10
@RalfJung
Copy link
Member

As announced before, this ran into the recent floating point changes, so now it does not build any more. I'm afraid I'll have to ask you to rebase. Also see rust-lang/rust#61673 and #764.

@kungfukennyg
Copy link
Contributor Author

Okay no worries. I've done the rebase, now I need some guidance on what to do next. rustc_apfloat doesn't yet have functions for cbrt and hypot.

@RalfJung
Copy link
Member

RalfJung commented Jun 12, 2019

Right, so please copy-paste what sin and friends do to convert to host-floats (including copying the FIXME), and then just call some host implementation like you did before.

@kungfukennyg
Copy link
Contributor Author

Okay I see, thanks!

@RalfJung
Copy link
Member

Forgot to add a link to that code, here it is.

@kungfukennyg
Copy link
Contributor Author

Should I move the cbrt and hypot match arms fromfn_call.rs to the others you've just linked in intrinsics.rs? That seems like the more fitting place.

@RalfJung
Copy link
Member

No, "intrinsics.rs" is for intrinsics and "fn_call" is for emulating C functions.

@kungfukennyg
Copy link
Contributor Author

Okay I see.

@kungfukennyg kungfukennyg force-pushed the fix_cmath_functions branch from 233a28d to 7c29de1 Compare June 12, 2019 19:43
Test cases are added to `tests/run-pass/intrinsics-math.rs`
@kungfukennyg kungfukennyg force-pushed the fix_cmath_functions branch from 7c29de1 to 535914e Compare June 12, 2019 19:44
@RalfJung
Copy link
Member

Yes this looks good. :) Let's see what CI says.

@RalfJung
Copy link
Member

CI is happy. :D Thanks a lot!

@RalfJung RalfJung merged commit b597eab into rust-lang:master Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants