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

f16 and f128 step 4: basic library support #122470

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Mar 14, 2024

This is the next step after #121926, another portion of #114607

Tracking issue: #116909

This PR adds the most basic operations to f16 and f128 that get lowered as LLVM intrinsics. This is a very small step but it seemed reasonable enough to add unopinionated basic operations before the larger modules that are built on top of them.

r? @Amanieu since you were pretty involved in the RFC
cc @compiler-errors
@rustbot label +T-libs-api +S-blocked +F-f16_and_f128

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 14, 2024
library/core/src/marker.rs Outdated Show resolved Hide resolved
tests/codegen/float/f16.rs Outdated Show resolved Hide resolved
@tgross35
Copy link
Contributor Author

@bjorn3 @antoyo do both GCC and Clif have CI covered enough that breaking library builds isn't a concern, as long as CI passes?

Contrasting this to #114607 (comment) which is from a while ago. Most of the inlines needed there were for formatting, which I do not intend to add until a few weeks from now anyway.

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Mar 14, 2024

do both GCC and Clif have CI covered enough that breaking library builds isn't a concern, as long as CI passes?

cg_clif doesn't build the standard library in CI. It reuses the one built by LLVM, so no CI passing is not enough. You also have to set cranelift as sole codegen backend in config.toml and then try a build to make it compile the standard library with cg_clif.

@antoyo
Copy link
Contributor

antoyo commented Mar 14, 2024

@bjorn3 @antoyo do both GCC and Clif have CI covered enough that breaking library builds isn't a concern, as long as CI passes?

I would assume it is the same as cg_cranelift, but cc-ing @GuillaumeGomez since he added the CI for cg_gcc.

@GuillaumeGomez
Copy link
Member

Just for information, we run a small subset of tests currently. You can see them here. I can add more if you want?

@tgross35
Copy link
Contributor Author

Thanks all for the info. Is there any reason the backends couldn't try building the libraries in CI, even if tests can't be run for any reason?

I can check locally but it would probably be good to have that feedback readily available, even beyond this change

@tgross35 tgross35 force-pushed the f16-f128-step4-libs-min branch 2 times, most recently from 4ef0a5f to 5bdb9dd Compare March 14, 2024 18:26
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the f16-f128-step4-libs-min branch from 5bdb9dd to fe4d151 Compare March 16, 2024 05:35
@tgross35
Copy link
Contributor Author

#121926 landed so I rebased, LLVM tests pass now. I cannot seem to get the different backends working though - running ./x t --stage 2 library/core/ library/std with codegen-backends = ["llvm", "cranelift"] passes, but I can't get cranelift alone to compile. These errors look more like a config issue than something caused by the changes in this PR, e.g. with codegen-backends = ["cranelift"] I get:

error: unsupported builtin codegen backend `llvm`

error: unsupported builtin codegen backend `llvm`

error: Failed to assemble `.globl __inline_asm_compiler_builtins__df7f94933c94f82c_cgu__000_n0
       .type __inline_asm_compiler_builtins__df7f94933c94f82c_cgu__000_n0,@function
       .section .text.__inline_asm_compiler_builtins__df7f94933c94f82c_cgu__000_n0,"ax",@progbits
       __inline_asm_compiler_builtins__df7f94933c94f82c_cgu__000_n0:
       .intel_syntax noprefix
...

And with codegen-backends = ["cranelift", "llvm"]:

   Compiling core v0.0.0 (/home/tmgross/projects/rust/rustc/library/core)
    Finished release [optimized] target(s) in 23.37s
     Running tests/lib.rs (build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/coretests-c2a4ac97d8e0ebe7)

running 1666 tests
fatal runtime error: failed to initiate panic, error 5
error: test failed, to rerun pass `-p core --test coretests`

Caused by:
  process didn't exit successfully: `/home/tmgross/projects/rust/rustc/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/coretests-c2a4ac97d8e0ebe7 --quiet -Z unstable-options --format json` (signal: 6, SIGABRT: process abort signal)
Build completed unsuccessfully in 0:05:24

I didn't yet try the GCC backend. Is there a more automated way to test with GCC now that it is in tree, like with the LLVM submodule? Or does it still need to be built separately

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

I didn't yet try the GCC backend. Is there a more automated way to test with GCC now that it is in tree, like with the LLVM submodule? Or does it still need to be built separately

Still needs to be built separately for now. Once the vendoring PR is merged, I'll be able to start adding config so you can specify which backend to use.

@tgross35 tgross35 force-pushed the f16-f128-step4-libs-min branch from fe4d151 to 73c80c6 Compare March 18, 2024 20:36
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 20, 2024

☔ The latest upstream changes (presumably #122754) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35 tgross35 force-pushed the f16-f128-step4-libs-min branch from 73c80c6 to 6d2c0cf Compare March 26, 2024 08:04
@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor Author

@rustbot label +rla-silenced

@rustbot rustbot added the rla-silenced Silences rust-log-analyzer postings to the PR it's added on. label Mar 26, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#122470 (`f16` and `f128` step 4: basic library support)
 - rust-lang#122954 (Be more specific when flagging imports as redundant due to the extern prelude)
 - rust-lang#123314 (Skip `unused_parens` report for `Paren(Path(..))` in macro.)
 - rust-lang#123360 (Document restricted_std)
 - rust-lang#123661 (Stabilize `cstr_count_bytes`)
 - rust-lang#123703 (Use `fn` ptr signature instead of `{closure@..}` in infer error)
 - rust-lang#123756 (clean up docs for `File::sync_*`)
 - rust-lang#123761 (Use `suggest_impl_trait` in return type suggestion on type error)

r? `@ghost`
`@rustbot` modify labels: rollup
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors bors merged commit aac3f24 into rust-lang:master Apr 11, 2024
11 of 12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2024
Rollup merge of rust-lang#122470 - tgross35:f16-f128-step4-libs-min, r=Amanieu

`f16` and `f128` step 4: basic library support

This is the next step after rust-lang#121926, another portion of rust-lang#114607

Tracking issue: rust-lang#116909

This PR adds the most basic operations to `f16` and `f128` that get lowered as LLVM intrinsics. This is a very small step but it seemed reasonable enough to add unopinionated basic operations before the larger modules that are built on top of them.

r? ```@Amanieu``` since you were pretty involved in the RFC
cc ```@compiler-errors```
```@rustbot``` label +T-libs-api +S-blocked +F-f16_and_f128
@tgross35 tgross35 deleted the f16-f128-step4-libs-min branch April 11, 2024 03:01
@compiler-errors
Copy link
Member

@craterbot run mode=check-only start=master#aa067fb984d36462548bb785da221bfaf38253f0 end=try#530e69408e76a29e47f25e5aedaf0e3fdf629e5b

@craterbot
Copy link
Collaborator

👌 Experiment pr-122470 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 12, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-122470 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Mark-Simulacrum
Copy link
Member

@craterbot abort

@craterbot
Copy link
Collaborator

🗑️ Experiment pr-122470 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 13, 2024
@Mark-Simulacrum
Copy link
Member

@craterbot run mode=check-only start=master#aa067fb984d36462548bb785da221bfaf38253f0 end=try#707d6562f55b55a7aab4ab3a904997116b358314

Trying with an actually existing commit from the unrolled build.

@craterbot
Copy link
Collaborator

👌 Experiment pr-122470 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-122470 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-122470 is completed!
📊 1691 regressed and 1 fixed (437764 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 17, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2024
Re-add `From<f16> for f64`

This impl was originally added in rust-lang#122470 before being removed in rust-lang#123830 due to rust-lang#123831. However, the issue only affects `f32` (which currently only has one `From<{float}>` impl, `From<f32>`) as `f64` already has two `From<{float}>` impls (`From<f32>` and `From<f64>`) and is also the float literal fallback type anyway. Therefore it is safe to re-add `From<f16> for f64`.

This PR also updates the FIXME link to point to the open issue rust-lang#123831 rather than the closed issue rust-lang#123824.

Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128 +T-libs-api
bors added a commit to rust-lang-ci/rust that referenced this pull request May 16, 2024
Re-add `From<f16> for f64`

This impl was originally added in rust-lang#122470 before being removed in rust-lang#123830 due to rust-lang#123831. However, the issue only affects `f32` (which currently only has one `From<{float}>` impl, `From<f32>`) as `f64` already has two `From<{float}>` impls (`From<f32>` and `From<f64>`) and is also the float literal fallback type anyway. Therefore it is safe to re-add `From<f16> for f64`.

This PR also updates the FIXME link to point to the open issue rust-lang#123831 rather than the closed issue rust-lang#123824.

Tracking issue: rust-lang#116909

`@rustbot` label +F-f16_and_f128 +T-libs-api
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)]` rla-silenced Silences rust-log-analyzer postings to the PR it's added on. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.