-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 stubs in IR and ABI for f16
and f128
#121728
Conversation
This PR changes Stable MIR cc @oli-obk, @celinval, @spastorino, @ouz-a Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in match checking cc @Nadrieril Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor The Miri subtree was changed cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in src/librustdoc/clean/types.rs cc @camelid Some changes occurred in exhaustiveness checking cc @Nadrieril |
c126076
to
44c1336
Compare
@@ -778,8 +778,10 @@ pub fn mir_to_const<'tcx>(lcx: &LateContext<'tcx>, result: mir::Const<'tcx>) -> | |||
let range = alloc_range(offset + size * idx, size); | |||
let val = alloc.read_scalar(&lcx.tcx, range, /* read_provenance */ false).ok()?; | |||
res.push(match flt { | |||
FloatTy::F16 => unimplemented!("f16_f128"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit wary about adding this, especially to tools. There's definitely a risk of forgetting some of them, causing ICEs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this, what would be better?
My original change did more https://github.com/rust-lang/rust/pull/114607/files#diff-83cfffe9dbc2758ecf7b34a433525befc32ebff4525aef0de105fa5e7b659c21R808-R815, but since the surface area of this PR is intentionally much smaller, the primitives aren't available for use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the benefit of small PRs and this comment isn't meant to block the PR. Just a concern, that we shouldn't forget about those unimplemented!
macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the panics could be added as a step to the tracking issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fairness to @tgross35, this is impossible to hit right now. I think this approach is the best for the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a todo list that mentions these at #116909 (comment)
@@ -0,0 +1,37 @@ | |||
// Make sure we don't ICE while incrementally adding f16 and f128 support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think parser is quite right, the parser doesn't care :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this test does literally nothing lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There didn't seem to be a good folder :) I can move it if there is a better place
I know it does nothing but I just wanted a test that can be updated as the steps move along. So as to not accidentally go from "f128 does nothing" to "typing f128 anywhere in your code causes an ICE" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tiny nits, then I think this is fine
FloatTy::F16 => unimplemented!("f16_f128"), | ||
FloatTy::F32 => 'f', | ||
FloatTy::F64 => 'd', | ||
FloatTy::F128 => unimplemented!("f16_f128"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rcvalle: are there encodings for these types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some more discussion for these at #114607 (comment) and rust-lang/rustc-demangle#64 for this too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replied on the other issue, but will reply it here as well for easier reference. My recommendation is:
- For
f128
, if it's compatible with__float128
regardless of the underlying representation (i.e., changes between IEEE 754R 128-bit floating point type or IBM extended double depending on the target), we should always encode it as 'g' as it will be how it will also be encoded by Clang. (FWIW, it seems Clang uses IEEE 754R 128-bit floating point type instead of IBM extended double for__float128
by default for ppc64le since May 2023 and__ibm128
also doesn't seem to have a distinct encoding specified in the Itanium C++ ABI mangling.) - For
f16
, if it's compatible with IEEE 754R 128-bit floating point type (__fp16
and_Float16
in Clang), we should always encode it as 'Dh'; if it's compatible with bfloat16 (which is unlikely?), we should always encode it as 'DF16b'. - The 'Dd, 'De', and 'Df' encodings are for when the Rust compiler and Clang support decimal floating point types.
tl;dr.: We should always encode f128
as 'g', and f16
as 'Dh'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note that this independent/dissociated to Rust mangling/demangling and a different encoding can be used for it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will update these in the upcoming PR to add intrinsics.
Total miss that they didn't make __float128
mangle as q
to be consistent with f
and d
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! :)
@@ -778,8 +778,10 @@ pub fn mir_to_const<'tcx>(lcx: &LateContext<'tcx>, result: mir::Const<'tcx>) -> | |||
let range = alloc_range(offset + size * idx, size); | |||
let val = alloc.read_scalar(&lcx.tcx, range, /* read_provenance */ false).ok()?; | |||
res.push(match flt { | |||
FloatTy::F16 => unimplemented!("f16_f128"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fairness to @tgross35, this is impossible to hit right now. I think this approach is the best for the moment.
@@ -0,0 +1,37 @@ | |||
// Make sure we don't ICE while incrementally adding f16 and f128 support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this test does literally nothing lol
44c1336
to
2dc026a
Compare
@rustbot label +F-f16_and_f128 |
…itive` Make changes necessary to support these types in the compiler.
2dc026a
to
406790e
Compare
@bors r+ rollup=never |
Seems like bors might be taking a nap https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Infinite.20redirect.20at.20.60bors.2Erust-lang.2Eorg.60 |
@bors r=compiler-errors rollup=never |
@bors r+ I believe the PR was already in the queue last time I checked this morning (until it was r-'d). This PR was not given a priority, and rollup=never PRs are merged in order of the date the PR was opened, which is why the current one is merging. |
🤦♂️ I thought it was approval date not opening date, thanks All the same something is weird. Every time #121242 got to the top of the queue in the past 24 hours Bors would give up and stop merging things, this one was right behind it. And it merged a stale commit of mine #119748 (comment) |
☀️ Test successful - checks-actions |
Finished benchmarking commit (6cbf092): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 650.561s -> 653.924s (0.52%) |
…, r=compiler-errors `f16` and `f128` step 2: intrinsics Continuation of rust-lang#121728, another portion of rust-lang#114607. This PR adds `f16` and `f128` intrinsics, and hooks them up to both HIR and LLVM. This is all still unexposed to the frontend, which will probably be the next step. Also update itanium mangling per `@rcvalle's` in https://github.com/rust-lang/rust/pull/121728/files#r1506570300, and fix a typo from step 1. Once these types are usable in code, I will add the codegen tests from rust-lang#114607 (codegen is passing on that branch) This does add more `unimplemented!`s to Clippy, but I still don't think we can do better until library support is added. r? `@compiler-errors` cc `@Nilstrieb` `@rustbot` label +T-compiler +F-f16_and_f128
Rollup merge of rust-lang#121841 - tgross35:f16-f128-step2-intrinsics, r=compiler-errors `f16` and `f128` step 2: intrinsics Continuation of rust-lang#121728, another portion of rust-lang#114607. This PR adds `f16` and `f128` intrinsics, and hooks them up to both HIR and LLVM. This is all still unexposed to the frontend, which will probably be the next step. Also update itanium mangling per `@rcvalle's` in https://github.com/rust-lang/rust/pull/121728/files#r1506570300, and fix a typo from step 1. Once these types are usable in code, I will add the codegen tests from rust-lang#114607 (codegen is passing on that branch) This does add more `unimplemented!`s to Clippy, but I still don't think we can do better until library support is added. r? `@compiler-errors` cc `@Nilstrieb` `@rustbot` label +T-compiler +F-f16_and_f128
…r=compiler-errors Add stubs in IR and ABI for `f16` and `f128` This is the very first step toward the changes in rust-lang#114607 and the [`f16` and `f128` RFC](https://rust-lang.github.io/rfcs/3453-f16-and-f128.html). It adds the types to `rustc_type_ir::FloatTy` and `rustc_abi::Primitive`, and just propagates those out as `unimplemented!` stubs where necessary. These types do not parse yet so there is no feature gate, and it should be okay to use `unimplemented!`. The next steps will probably be AST support with parsing and the feature gate. r? `@compiler-errors` cc `@Nilstrieb` suggested breaking the PR up in rust-lang#120645 (comment)
…ler-errors `f16` and `f128` step 2: intrinsics Continuation of rust-lang/rust#121728, another portion of rust-lang/rust#114607. This PR adds `f16` and `f128` intrinsics, and hooks them up to both HIR and LLVM. This is all still unexposed to the frontend, which will probably be the next step. Also update itanium mangling per `@rcvalle's` in https://github.com/rust-lang/rust/pull/121728/files#r1506570300, and fix a typo from step 1. Once these types are usable in code, I will add the codegen tests from #114607 (codegen is passing on that branch) This does add more `unimplemented!`s to Clippy, but I still don't think we can do better until library support is added. r? `@compiler-errors` cc `@Nilstrieb` `@rustbot` label +T-compiler +F-f16_and_f128
…r=compiler-errors Add stubs in IR and ABI for `f16` and `f128` This is the very first step toward the changes in rust-lang#114607 and the [`f16` and `f128` RFC](https://rust-lang.github.io/rfcs/3453-f16-and-f128.html). It adds the types to `rustc_type_ir::FloatTy` and `rustc_abi::Primitive`, and just propagates those out as `unimplemented!` stubs where necessary. These types do not parse yet so there is no feature gate, and it should be okay to use `unimplemented!`. The next steps will probably be AST support with parsing and the feature gate. r? `@compiler-errors` cc `@Nilstrieb` suggested breaking the PR up in rust-lang#120645 (comment)
Relevant upstream changes: rust-lang/rust#120675: An intrinsic `Symbol` is now wrapped in a `IntrinsicDef` struct, so the relevant part of the code needed to be updated. rust-lang/rust#121464: The second argument of the `create_wrapper_file` function changed from a vector to a string. rust-lang/rust#121662: `NullOp::DebugAssertions` was renamed to `NullOp::UbCheck` and it now has data (currently unused by Kani) rust-lang/rust#121728: Introduces `F16` and `F128`, so needed to add stubs for them rust-lang/rust#121969: `parse_sess` was renamed to `psess`, so updated the relevant code. rust-lang/rust#122059: The `is_val_statically_known` intrinsic is now used in some `core::fmt` code, so had to handle it in (codegen'ed to false). rust-lang/rust#122233: This added a new `retag_box_to_raw` intrinsic. This is an operation that is primarily relevant for stacked borrows. For Kani, we just return the pointer. Resolves #3057
Remove `f16` and `f128` ICE paths from smir Just chasing down some possible ICE paths. `@compiler-errors` mentioned in rust-lang#121728 (comment) that it is okay not to support these in smir, but this change seems pretty trivial? r? `@celinval` since you reviewed rust-lang#114607 (review)
Remove `f16` and `f128` ICE paths from smir Just chasing down some possible ICE paths. ``@compiler-errors`` mentioned in rust-lang#121728 (comment) that it is okay not to support these in smir, but this change seems pretty trivial? r? ``@celinval`` since you reviewed rust-lang#114607 (review)
Remove `f16` and `f128` ICE paths from smir Just chasing down some possible ICE paths. ```@compiler-errors``` mentioned in rust-lang#121728 (comment) that it is okay not to support these in smir, but this change seems pretty trivial? r? ```@celinval``` since you reviewed rust-lang#114607 (review)
Rollup merge of rust-lang#126983 - tgross35:f16-f128-smir, r=celinval Remove `f16` and `f128` ICE paths from smir Just chasing down some possible ICE paths. ```@compiler-errors``` mentioned in rust-lang#121728 (comment) that it is okay not to support these in smir, but this change seems pretty trivial? r? ```@celinval``` since you reviewed rust-lang#114607 (review)
This is the very first step toward the changes in #114607 and the
f16
andf128
RFC. It adds the types torustc_type_ir::FloatTy
andrustc_abi::Primitive
, and just propagates those out asunimplemented!
stubs where necessary.These types do not parse yet so there is no feature gate, and it should be okay to use
unimplemented!
.The next steps will probably be AST support with parsing and the feature gate.
r? @compiler-errors
cc @Nilstrieb suggested breaking the PR up in #120645 (comment)