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 f16 and f128 const eval for binary and unary operationations #126429

Merged
merged 4 commits into from
Jun 15, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jun 13, 2024

Add const evaluation and Miri support for f16 and f128, including unary and binary operations. Casts are not yet included.

Fixes #124583

r? @RalfJung

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Jun 13, 2024

src/tools/miri/tests/pass/float.rs is really tedious to update and I'm not entirely clear what a lot of the magic numbers are intended to test. @RalfJung do you have any suggestions about how this could be cleaned up or made generic to some extent? Compiler-builtins uses a bit of fuzzing + property testing to compare the builtins function to the system function or apfloat https://github.com/rust-lang/compiler-builtins/blob/c04eb9e1afb72bdf943f5e5d77b3812f40526602/testcrate/src/lib.rs#L255, but we don't really have anything to compare the result to here.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Jun 13, 2024

The file is kind of grown over time, and I did at some point copy a whole bunch of corner case tests over from the rustc test suite -- most of the cast tests are from that, I think. So I don't always know the point of the magic numbers either, I just hoped whoever who wrote the rustc tests knew.^^

But I don't know a good way to make this generic, no.

@tgross35
Copy link
Contributor Author

That makes more sense if they came from the rust suite. I just removed the const eval casting until I can take a closer look at that and hopefully clean up that test a bit. Having only unary and binary operations still saves a ton of the headache that was blocking me elsewhere (ICE on negative numbers 😠)

Ready for a look when you get the chance.

@tgross35 tgross35 marked this pull request as ready for review June 13, 2024 20:35
@rustbot
Copy link
Collaborator

rustbot commented Jun 13, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Comment on lines -1 to -5
//@ known-bug: rust-lang/rust#124583

fn main() {
let _ = -(-0.0f16);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add Fixes #124583 to pr header so that this is auto closed? thx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it in a commit message and thought it would snag it there, but added just in case :)

Copy link
Member

Choose a reason for hiding this comment

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

ah I didn't check the commits 🙃 should be good enough then :)

@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor Author

Spurious failure: #126430

Comment on lines 41 to 46

#[test]
fn test_neg_zero() {
// only verify const eval negation until we add the rest of the checks
let _neg_zero: f16 = -0.0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These are thoroughly covered by the miri tests, I don't think we need it here, but if you wanna keep it, this needs to be f128

Copy link
Member

Choose a reason for hiding this comment

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

Also this isn't even doing any const-eval? If anything it might be hitting some mir-opt codepaths.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, const prop was hitting the mir interp code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks for the catch, I missed that it was double negation that caused the crash in #124583. Updated tests/ui/numbers-arithmetic/f16-f128-lit.rs instead

@tgross35 tgross35 force-pushed the f16-f128-const-eval branch 2 times, most recently from 80c32ca to df2f8f7 Compare June 14, 2024 17:30
float_to_int_fallback!(f16 => i8, u8, i16, u16, i32, u32, i64, u64, i128, u128);
float_to_int!(f32=> i8, u8, i16, u16, i32, u32, i64, u64, i128, u128);
float_to_int!(f64 => i8, u8, i16, u16, i32, u32, i64, u64, i128, u128);
float_to_int_fallback!(f128 => i8, u8, i16, u16, i32, u32, i64, u64, i128, u128);
Copy link
Member

Choose a reason for hiding this comment

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

The new impls aren't actually used, are they?

Copy link
Contributor Author

@tgross35 tgross35 Jun 14, 2024

Choose a reason for hiding this comment

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

Not without the cast changes that I wound up removing, I left the switch to the macro but removed f16/f128.

@RalfJung
Copy link
Member

The last commit seems to un-do a bunch of the changes from the earlier commits...?

This excludes casting, which needs more tests.
Const eval negation was added. This test is now covered by Miri tests,
and merged into an existing UI test.

Fixes <rust-lang#124583>
@tgross35
Copy link
Contributor Author

The last commit seems to un-do a bunch of the changes from the earlier commits...?

I meant to remove the changes to cast entirely since I don't have a good test (getting tedious as mentioned above), just did a bad job rebasing. History should be cleaner now.

FloatTy::F32 =>
float_to_int_inner::<Single>(this, src.to_scalar().to_f32()?, cast_to, round),
FloatTy::F64 =>
float_to_int_inner::<Double>(this, src.to_scalar().to_f64()?, cast_to, round),
FloatTy::F128 => unimplemented!("f16_f128"),
FloatTy::F128 =>
float_to_int_inner::<Quad>(this, src.to_scalar().to_f128()?, cast_to, round),
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be dead code now that you removed the cast tests?

OTOH, the code is rather obvious, so I am also fine with keeping it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you are right. If you have no problem then I think I may as well just leave it, casts won't be far behind.

@RalfJung
Copy link
Member

Maybe update the PR title and description to say what is covered? Mainly unops and binops, it seems.

r=me after that, with or without the helpers.rs change.

@tgross35 tgross35 changed the title Add f16 and f128 const evaluation Add f16 and f128 const eval for binary and unary operationations Jun 14, 2024
@tgross35
Copy link
Contributor Author

I updated the description, feel free to tweak it if I'm missing something. (You or someone else need to r for me).

@rustbot label +F-f16_and_f128

@rustbot rustbot added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Jun 14, 2024
@RalfJung
Copy link
Member

RalfJung commented Jun 14, 2024 via email

@bors
Copy link
Contributor

bors commented Jun 14, 2024

📌 Commit e649042 has been approved by RalfJung

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 15, 2024
…lfJung

Add `f16` and `f128` const eval for binary and unary operationations

Add const evaluation and Miri support for f16 and f128, including unary and binary operations. Casts are not yet included.

Fixes rust-lang#124583

r? `@RalfJung`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#125829 (rustc_span: Add conveniences for working with span formats)
 - rust-lang#126279 (Migrate `inaccessible-temp-dir`, `output-with-hyphens` and `issue-10971-temps-dir` `run-make` tests to `rmake`)
 - rust-lang#126361 (Unify intrinsics body handling in StableMIR)
 - rust-lang#126417 (Add `f16` and `f128` inline ASM support for `x86` and `x86-64`)
 - rust-lang#126424 ( Also sort `crt-static` in `--print target-features` output)
 - rust-lang#126428 (Polish `std::path::absolute` documentation.)
 - rust-lang#126429 (Add `f16` and `f128` const eval for binary and unary operationations)
 - rust-lang#126448 (End support for Python 3.8 in tidy)
 - rust-lang#126488 (Use `std::path::absolute` in bootstrap)
 - rust-lang#126511 (.mailmap: Associate both my work and my private email with me)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#125829 (rustc_span: Add conveniences for working with span formats)
 - rust-lang#126361 (Unify intrinsics body handling in StableMIR)
 - rust-lang#126417 (Add `f16` and `f128` inline ASM support for `x86` and `x86-64`)
 - rust-lang#126424 ( Also sort `crt-static` in `--print target-features` output)
 - rust-lang#126428 (Polish `std::path::absolute` documentation.)
 - rust-lang#126429 (Add `f16` and `f128` const eval for binary and unary operationations)
 - rust-lang#126448 (End support for Python 3.8 in tidy)
 - rust-lang#126488 (Use `std::path::absolute` in bootstrap)
 - rust-lang#126511 (.mailmap: Associate both my work and my private email with me)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3775f2f into rust-lang:master Jun 15, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 15, 2024
Rollup merge of rust-lang#126429 - tgross35:f16-f128-const-eval, r=RalfJung

Add `f16` and `f128` const eval for binary and unary operationations

Add const evaluation and Miri support for f16 and f128, including unary and binary operations. Casts are not yet included.

Fixes rust-lang#124583

r? ``@RalfJung``
@tgross35 tgross35 deleted the f16-f128-const-eval branch June 18, 2024 04:16
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2024
…Simulacrum

Add more constants, functions, and tests for `f16` and `f128`

This adds everything that was in some way blocked on const eval, since rust-lang#126429 landed. There is a lot of `cfg(bootstrap)` since that is a fairly recent change.

`f128` tests are disabled on everything except x86_64 and Linux aarch64, which are two platforms I know have "good" support for these types - meaning basic math symbols are available and LLVM doesn't hit selection crashes. `f16` tests are enabled on almost everything except for known LLVM crashes. Doctests are only enabled on x86_64.

Tracking issue: rust-lang#116909
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
…mulacrum

Add more constants, functions, and tests for `f16` and `f128`

This adds everything that was in some way blocked on const eval, since rust-lang#126429 landed. There is a lot of `cfg(bootstrap)` since that is a fairly recent change.

`f128` tests are disabled on everything except x86_64 and Linux aarch64, which are two platforms I know have "good" support for these types - meaning basic math symbols are available and LLVM doesn't hit selection crashes. `f16` tests are enabled on almost everything except for known LLVM crashes. Doctests are only enabled on x86_64.

Tracking issue: rust-lang#116909
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
…mulacrum

Add more constants, functions, and tests for `f16` and `f128`

This adds everything that was in some way blocked on const eval, since rust-lang#126429 landed. There is a lot of `cfg(bootstrap)` since that is a fairly recent change.

`f128` tests are disabled on everything except x86_64 and Linux aarch64, which are two platforms I know have "good" support for these types - meaning basic math symbols are available and LLVM doesn't hit selection crashes. `f16` tests are enabled on almost everything except for known LLVM crashes. Doctests are only enabled on x86_64.

Tracking issue: rust-lang#116909
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 27, 2024
Add more constants, functions, and tests for `f16` and `f128`

This adds everything that was in some way blocked on const eval, since rust-lang/rust#126429 landed. There is a lot of `cfg(bootstrap)` since that is a fairly recent change.

`f128` tests are disabled on everything except x86_64 and Linux aarch64, which are two platforms I know have "good" support for these types - meaning basic math symbols are available and LLVM doesn't hit selection crashes. `f16` tests are enabled on almost everything except for known LLVM crashes. Doctests are only enabled on x86_64.

Tracking issue: rust-lang/rust#116909
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
Add more constants, functions, and tests for `f16` and `f128`

This adds everything that was in some way blocked on const eval, since rust-lang/rust#126429 landed. There is a lot of `cfg(bootstrap)` since that is a fairly recent change.

`f128` tests are disabled on everything except x86_64 and Linux aarch64, which are two platforms I know have "good" support for these types - meaning basic math symbols are available and LLVM doesn't hit selection crashes. `f16` tests are enabled on almost everything except for known LLVM crashes. Doctests are only enabled on x86_64.

Tracking issue: rust-lang/rust#116909
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: Invalid float op Neg
7 participants