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

APFloat: Rewrite It In Rust and use it for deterministic floating-point CTFE. #43554

Merged
merged 6 commits into from
Aug 5, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jul 30, 2017

As part of the CTFE initiative, we're forced to find a solution for floating-point operations.
By design, IEEE-754 does not explicitly define everything in a deterministic manner, and there is some variability between platforms, at the very least (e.g. NaN payloads).

If types are to evaluate constant expressions involving type (or in the future, const) generics, that evaluation needs to be fully deterministic, even across rustc host platforms.
That is, if [T; T::X] was used in a cross-compiled library, and the evaluation of T::X executed a floating-point operation, that operation has to be reproducible on any other host, only knowing T and the definition of the X associated const (as either AST or HIR).

Failure to uphold those rules allows an associated type (e.g. <Foo as Iterator>::Item) to be seen as two (or more) different types, depending on the current host, and such type safety violations typically allow writing of a transmute in safe code, given enough generics.

The options considered by @rust-lang/compiler were:

  1. Ban floating-point operations in generic const-evaluation contexts
  2. Emulate floating-point operations in an uniformly deterministic fashion

The former option may seem appealing at first, but floating-point operations are allowed today, so they can't be banned wholesale, a distinction has to be made between the code that already works, and future generic contexts. Moreover, every computation that succeeded has to be cached, otherwise the generic case can be reproduced without any generics. IMO there are too many ways it can go wrong, and a single violation can be enough for an unsoundness hole.
Not to mention we may end up really wanting floating-point operations anyway, in CTFE.

I went with the latter option, and seeing how LLVM already has a library for this exact purpose (as it needs to perform optimizations independently of host floating-point capabilities), i.e. APFloat, that was what I ended up basing this PR on.
But having been burned by the low reusability of bindings that link to LLVM, and because I would rather the floating-point operations to be wrong than not deterministic or not memory-safe (APFloat does far more pointer juggling than I'm comfortable with), I decided to RIIR.

This way, we have a guarantee of no unsafe code, a bit more control over the where native floating-point might accidentally be involved, and non-LLVM backends can share it.
I've also ported all the testcases over, before any functionality, to catch any mistakes.

Currently the PR replaces all CTFE operations to go through apfloat::ieee::{Single,Double}, keeping only the bits of the f32 / f64 memory representation in between operations.
Converting from a string also double-checks that core::num and apfloat agree on the interpretation of a floating-point number literal, in case either of them has any bugs left around.

r? @nikomatsakis
f? @nagisa @est31


Huge thanks to @edef1c for first demoing usable APFloat bindings and to @chandlerc for fielding my questions on IRC about APFloat peculiarities (also upstreaming some bugfixes).

@eddyb eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jul 30, 2017
@eddyb eddyb requested a review from nikomatsakis July 30, 2017 07:27
@eddyb
Copy link
Member Author

eddyb commented Jul 30, 2017

cc @solson @oli-obk @RalfJung Any thoughts on miri switching fully to this? The API isn't that ergonomic (if you always have to go from/to bits) but we could figure something out.

@eddyb
Copy link
Member Author

eddyb commented Jul 30, 2017

Started a crater run (although cargobomb would be more useful).

@Mark-Simulacrum
Copy link
Member

If you can update the PR to fix the test case and run try for bors that'll help get the crater run started.

[01:00:47] 	thread 'apfloat::tests::to_string' panicked at 'NYI fmt_custom', /checkout/src/librustc_const_math/apfloat.rs:1680:8

@eddyb
Copy link
Member Author

eddyb commented Jul 30, 2017

@Mark-Simulacrum You mean cargobomb? I already have crater running, it doesn't run tests.

EDIT: Crater report shows no regressions.

@eddyb eddyb removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jul 30, 2017
@Mark-Simulacrum
Copy link
Member

Yeah, I meant cargobomb.

@alexcrichton
Copy link
Member

It looks like 8kloc of the 11kloc file is unit tests (awesome!), in which case maybe that'd be good for a src/librustc_const_eval/tests/foo.rs test? That way if you edit tests you won't have to rebootstrap the compiler!

AFAIK all you need to do is copy the info to tests/foo.rs and everything will "just work" b/c it's a "standard" cargo layout.

@eddyb
Copy link
Member Author

eddyb commented Jul 30, 2017

@alexcrichton Ahh the advanced technology of cargo test! That'll come in real handy.

};
C_floating_f64(v_f64, llty)
consts::bitcast(Const::from_constint(ccx, &bits).llval, llty)
Copy link
Member Author

Choose a reason for hiding this comment

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

@alexcrichton Was there a concern at some point that passing signaling NaNs to LLVM could trigger an exception on the host? Mostly wondering in terms of marking it as a solved problem.

Copy link
Member

Choose a reason for hiding this comment

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

Hm I don't recall any particular concerns about signaling NaNs and CTFE on the host, no. I'd think it's roughly ok to basically gloss over them though.

@RalfJung
Copy link
Member

Any thoughts on miri switching fully to this? The API isn't that ergonomic (if you always have to go from/to bits) but we could figure something out.

I don't know much about floating point, and have no strong opinion on how to handle them. Rust-only softfloat sounds very reasonable.

One thing I am wodnering about this PR in general -- why is APFloat.rs not a separate crate on crates.io? Compilers may not be the only ones wanting deterministic floating point.

@eddyb
Copy link
Member Author

eddyb commented Jul 30, 2017

@RalfJung Having started as part of an optimizing compiler, it's geared towards emulation of IEEE and PPC double-double as well as possible (not everything is complete, especially the PPC side).
Other applications might want a truly arbitrary-precision library instead.
I'm not against putting this on crates.io but I'm not sure it's wise to do it right away. I will happily place it in an rustc_apfloat crate of its own if that is desired.

@eddyb
Copy link
Member Author

eddyb commented Jul 30, 2017

@bors try

@bors
Copy link
Contributor

bors commented Jul 30, 2017

⌛ Trying commit 08d9e09 with merge 21ee79b...

bors added a commit that referenced this pull request Jul 30, 2017
APFloat: Rewrite It In Rust and use it for deterministic floating-point CTFE.

As part of the CTFE initiative, we're forced to find a solution for floating-point operations.
By design, IEEE-754 does not explicitly define everything in a deterministic manner, and there is some variability between platforms, at the very least (e.g. NaN payloads).

If types are to evaluate constant expressions involving type (or in the future, const) generics, that evaluation needs to be *fully deterministic*, even across `rustc` host platforms.
That is, if `[T; T::X]` was used in a cross-compiled library, and the evaluation of `T::X` executed a floating-point operation, that operation has to be reproducible on *any other host*, only knowing `T` and the definition of the `X` associated const (as either AST or HIR).

Failure to uphold those rules allows an associated type (e.g. `<Foo as Iterator>::Item`) to be seen as two (or more) different types, depending on the current host, and such type safety violations typically allow writing of a `transmute` in safe code, given enough generics.

The options considered by @rust-lang/compiler were:
1. Ban floating-point operations in generic const-evaluation contexts
2. Emulate floating-point operations in an uniformly deterministic fashion

The former option may seem appealing at first, but floating-point operations *are allowed today*, so they can't be banned wholesale, a distinction has to be made between the code that already works, and future generic contexts. *Moreover*, every computation that succeeded *has to be cached*, otherwise the generic case can be reproduced without any generics. IMO there are too many ways it can go wrong, and a single violation can be enough for an unsoundness hole.
Not to mention we may end up really wanting floating-point operations *anyway*, in CTFE.

I went with the latter option, and seeing how LLVM *already* has a library for this exact purpose (as it needs to perform optimizations independently of host floating-point capabilities), i.e. `APFloat`, that was what I ended up basing this PR on.
But having been burned by the low reusability of bindings that link to LLVM, and because I would *rather* the floating-point operations to be wrong than not deterministic or not memory-safe (`APFloat` does far more pointer juggling than I'm comfortable with), I decided to RIIR.

This way, we have a guarantee of *no* `unsafe` code, a bit more control over the where native floating-point might accidentally be involved, and non-LLVM backends can share it.
I've also ported all the testcases over, *before* any functionality, to catch any mistakes.

Currently the PR replaces all CTFE operations to go through `apfloat::{IeeeSingle, IeeeDouble}`, keeping only the bits of the `f32` / `f64` memory representation in between operations.
Converting from a string also double-checks that `core::num` and `apfloat` agree on the interpretation of a floating-point number literal, in case either of them has any bugs left around.

r? @nikomatsakis
f? @nagisa @est31

<hr/>

Huge thanks to @edef1c for first demoing usable `APFloat` bindings and to @chandlerc for fielding my questions on IRC about `APFloat` peculiarities (also upstreaming some bugfixes).

**TODO**:
* should tests live in a separate file?
* maybe get more tests from some IEEE conformance testsuite
* maybe go through the trouble of making a new crate
* maybe describe the porting process and the decisions made (towards a rusty bias)
* clean up the API a bit, the method names could be more uniform (e.g. `rounding_add`) and `OpStatus` could be more monadic (a struct instead of `(T, OpStatus)`)
* add licensing information (moco lawyers checked off on this a while back FWIW)
@bors
Copy link
Contributor

bors commented Jul 30, 2017

☀️ Test successful - status-travis
State: approved= try=True

@Mark-Simulacrum
Copy link
Member

@brson @tomprince @frewsxcv Can one of you schedule a cargobomb run? Try build completed.

@carols10cents carols10cents added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jul 31, 2017
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me if tests are separated out into their own file and cargobomb is happy.

This looks awesome. I admit I didn't really read the apfloat source, I'm trusting your tests. Skimming the compiler integration though it looks pretty smooth.

@eddyb
Copy link
Member Author

eddyb commented Aug 2, 2017

I've split the code into modules (plus two separate unit test files) and placed it in rustc_apfloat.
Are there licensing concerns? That is, given we already ship LLVM's license, do I need to, say, have a custom license header at the top of all the files in rustc_apfloat?
Maybe replace Copyright 2017 The Rust Project Developers with something else?
cc @aturon @brson

@eddyb
Copy link
Member Author

eddyb commented Aug 2, 2017

@bors try

If you haven't started a cargobomb run yet, please wait for this try to finish and use it instead.

@aturon
Copy link
Member

aturon commented Aug 2, 2017

@eddyb We're in the process of removing the copyright headers entirely. The section in the COPYRIGHT file is all that's needed.

Also: congrats on this huge accomplishment!

@tomprince
Copy link
Member

Cargobomb results

@eddyb
Copy link
Member Author

eddyb commented Aug 5, 2017

Thanks @tomprince! Currently waiting for all the logs to get uploaded.
Most of the failures I've seen so far are the same clippy compile error, and the rest are spurious (most of them are tests which have been written broken e.g. unsafe data races).

EDIT: all reports are uploaded, no true regression observed, proceeding...

@eddyb
Copy link
Member Author

eddyb commented Aug 5, 2017

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 5, 2017

📌 Commit c457b26 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 5, 2017

⌛ Testing commit c457b26 with merge 1ee04eadff357267f85c9af17d5935c1e5ea8ad7...

@bors
Copy link
Contributor

bors commented Aug 5, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 1ee04eadff357267f85c9af17d5935c1e5ea8ad7 to master...

@bors
Copy link
Contributor

bors commented Aug 5, 2017

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@eddyb
Copy link
Member Author

eddyb commented Aug 5, 2017

cc @rust-lang/infra about the strange push failure above.

@bors retry

@bors
Copy link
Contributor

bors commented Aug 5, 2017

⌛ Testing commit c457b26 with merge 2b82b7e...

bors added a commit that referenced this pull request Aug 5, 2017
APFloat: Rewrite It In Rust and use it for deterministic floating-point CTFE.

As part of the CTFE initiative, we're forced to find a solution for floating-point operations.
By design, IEEE-754 does not explicitly define everything in a deterministic manner, and there is some variability between platforms, at the very least (e.g. NaN payloads).

If types are to evaluate constant expressions involving type (or in the future, const) generics, that evaluation needs to be *fully deterministic*, even across `rustc` host platforms.
That is, if `[T; T::X]` was used in a cross-compiled library, and the evaluation of `T::X` executed a floating-point operation, that operation has to be reproducible on *any other host*, only knowing `T` and the definition of the `X` associated const (as either AST or HIR).

Failure to uphold those rules allows an associated type (e.g. `<Foo as Iterator>::Item`) to be seen as two (or more) different types, depending on the current host, and such type safety violations typically allow writing of a `transmute` in safe code, given enough generics.

The options considered by @rust-lang/compiler were:
1. Ban floating-point operations in generic const-evaluation contexts
2. Emulate floating-point operations in an uniformly deterministic fashion

The former option may seem appealing at first, but floating-point operations *are allowed today*, so they can't be banned wholesale, a distinction has to be made between the code that already works, and future generic contexts. *Moreover*, every computation that succeeded *has to be cached*, otherwise the generic case can be reproduced without any generics. IMO there are too many ways it can go wrong, and a single violation can be enough for an unsoundness hole.
Not to mention we may end up really wanting floating-point operations *anyway*, in CTFE.

I went with the latter option, and seeing how LLVM *already* has a library for this exact purpose (as it needs to perform optimizations independently of host floating-point capabilities), i.e. `APFloat`, that was what I ended up basing this PR on.
But having been burned by the low reusability of bindings that link to LLVM, and because I would *rather* the floating-point operations to be wrong than not deterministic or not memory-safe (`APFloat` does far more pointer juggling than I'm comfortable with), I decided to RIIR.

This way, we have a guarantee of *no* `unsafe` code, a bit more control over the where native floating-point might accidentally be involved, and non-LLVM backends can share it.
I've also ported all the testcases over, *before* any functionality, to catch any mistakes.

Currently the PR replaces all CTFE operations to go through `apfloat::ieee::{Single,Double}`, keeping only the bits of the `f32` / `f64` memory representation in between operations.
Converting from a string also double-checks that `core::num` and `apfloat` agree on the interpretation of a floating-point number literal, in case either of them has any bugs left around.

r? @nikomatsakis
f? @nagisa @est31

<hr/>

Huge thanks to @edef1c for first demoing usable `APFloat` bindings and to @chandlerc for fielding my questions on IRC about `APFloat` peculiarities (also upstreaming some bugfixes).
@bors
Copy link
Contributor

bors commented Aug 5, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2b82b7e to master...

@bors bors merged commit c457b26 into rust-lang:master Aug 5, 2017
@kennytm
Copy link
Member

kennytm commented Aug 5, 2017

cc #43535.

@coder543
Copy link

coder543 commented Aug 9, 2017

@eddyb This is another vote to have apfloat be a crates.io library. huonw/float is nice, but it depends on ramp, which seems to require nightly, and it looks like float itself might depend on nightly. Would apfloat require nightly?

It's also odd to name the library arbitrary precision float if it isn't truly arbitrary precision.

@eddyb
Copy link
Member Author

eddyb commented Aug 9, 2017

The IEEE support is configurable (but I chose to use types instead of runtime config), even from outside the crate - you can quite easily create a 24-bit IEEE-like floating-point type with it.
Also, I kept the name from LLVM.

I have nothing against it being on crates.io but that means more work on its API to be more ergonomic, and placing it in the nursery, which @rust-lang/libs gets to decide on.

ramp doesn't have opt-out for the nightly requirement? I thought it did.
As for this library, give me a minute to look at its nightly feature requirements.

#![feature(const_fn)]
#![feature(i128_type)]
#![feature(slice_patterns)]
#![feature(try_from)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Out of these 4 nightly features, the only hard one to remove is i128_type. The non-nightly version would have to be limited to 64-bit floats instead of supporting 128-bit (IEEE and PPC).

@coder543
Copy link

coder543 commented Aug 9, 2017

ramp doesn't have opt-out for the nightly requirement? I thought it did.

From the ramp README:

NOTE Due to use of unstable features (notably inline assembly), Ramp can only be compiled with a nightly build of rustc.

So, I don't think it does.

Out of these 4 nightly features, the only hard one to remove is i128_type.

Hopefully that'll be stabilized soonish.

@eddyb
Copy link
Member Author

eddyb commented Aug 9, 2017

Yeah for some reason when I looked at ramp's code, the assembly seemed opt-out.

@alexcrichton
Copy link
Member

@eddyb it looks like this caused a 30% regression in the tuple-stress test, do you know if that's a fluke on perf or perhaps related to this?

@alexcrichton
Copy link
Member

Er I went ahead and opened a dedicated issue for this -- #43828

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-crater Status: Waiting on a crater run to be completed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.