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 3: compiler support & feature gate #121926

Merged
merged 5 commits into from
Mar 16, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Mar 3, 2024

Continuation of #121841, another portion of #114607

This PR exposes the new types to the world and adds a feature gate. Marking this as a draft because I need some feedback on where I did the feature gate check. It also does not yet catch type via suffixed literals (so the feature gate test will fail, probably some others too because I haven't belssed).

If there is a better place to check all types after resolution, I can do that. If not, I figure maybe I can add a second gate location in AST when it checks numeric suffixes.

Unfortunately I still don't think there is much testing to be done for correctness (codegen tests or parsed value checks) until we have basic library support. I think that will be the next step.

Tracking issue: #116909

r? @compiler-errors
cc @Nilstrieb
@rustbot label +F-f16_and_f128

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` labels Mar 3, 2024
@tgross35 tgross35 force-pushed the f16-f128-step3-feature-gate branch from e509a89 to a7ce0b8 Compare March 3, 2024 03:09
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the f16-f128-step3-feature-gate branch from a7ce0b8 to 9954861 Compare March 4, 2024 04:13
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 4, 2024
@tgross35 tgross35 force-pushed the f16-f128-step3-feature-gate branch 3 times, most recently from 8f1dbe9 to 603aeb6 Compare March 4, 2024 04:59
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 force-pushed the f16-f128-step3-feature-gate branch 2 times, most recently from 0e2b72c to d27edfb Compare March 4, 2024 05:31
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Are there any positive tests (i.e. ones that show that feature(f16)/feature(f128) actually allows us to name and use f16/f128)?

compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_passes/src/feature_gate.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_passes/src/feature_gate.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/ident.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/ident.rs Outdated Show resolved Hide resolved
@tgross35 tgross35 force-pushed the f16-f128-step3-feature-gate branch from d27edfb to b8af79d Compare March 4, 2024 19:01
@tgross35 tgross35 marked this pull request as ready for review March 4, 2024 19:04
@rustbot
Copy link
Collaborator

rustbot commented Mar 4, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@tgross35
Copy link
Contributor Author

tgross35 commented Mar 4, 2024

Are there any positive tests (i.e. ones that show that feature(f16)/feature(f128) actually allows us to name and use f16/f128)?

I was thinking the feature gate test happened both with and without the gate but I guess not. Added tests/ui/resolve/primitive-usage.rs*

@tgross35 tgross35 force-pushed the f16-f128-step3-feature-gate branch from b8af79d to 1bc946b Compare March 4, 2024 19:14
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 4, 2024

📌 Commit 1bc946b has been approved by compiler-errors

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 Mar 4, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Mar 4, 2024

I think that's the last of the compiler side for a while 🎉 thanks for the ton of help figuring this all out @compiler-errors

@bors
Copy link
Contributor

bors commented Mar 15, 2024

📌 Commit 2098fec has been approved by compiler-errors,petrochenkov

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 Mar 15, 2024
@tgross35
Copy link
Contributor Author

Wouldn't doing the gate in rustc_ast_lowering rather than after resolution mean that user-defined types named f16 or f128 also get flagged?

User defined types will result in Res::Def, not Res::PrimTy.

I only see Res::PrimTy being constructed once in ast_lowering at

hir::TyKind::Path(hir::QPath::Resolved(
None,
self.arena.alloc(hir::Path {
res: Res::PrimTy(hir::PrimTy::Bool),
span,
segments: self.arena.alloc_from_iter([hir::PathSegment {
ident: Ident { name: sym::bool, span },
hir_id: bool_id,
res: Res::PrimTy(hir::PrimTy::Bool),
args: None,
infer_args: false,
}]),
}),
)),
, which looks to be for generics. The rest are in rustc_resolve/src/late.rs, which we intercept here.

Thanks for the r+. As-is should be workable for now then, I can later update this gating to your suggestion if it improves the situation with libs. I am just not exactly sure what this would look like still.

@matthiaskrgr
Copy link
Member

@bors rollup=iffy

@bors
Copy link
Contributor

bors commented Mar 15, 2024

⌛ Testing commit 2098fec with merge 76a9141...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
…, r=compiler-errors,petrochenkov

`f16` and `f128` step 3: compiler support & feature gate

Continuation of rust-lang#121841, another portion of rust-lang#114607

This PR exposes the new types to the world and adds a feature gate. Marking this as a draft because I need some feedback on where I did the feature gate check. It also does not yet catch type via suffixed literals (so the feature gate test will fail, probably some others too because I haven't belssed).

If there is a better place to check all types after resolution, I can do that. If not, I figure maybe I can add a second gate location in AST when it checks numeric suffixes.

Unfortunately I still don't think there is much testing to be done for correctness (codegen tests or parsed value checks) until we have basic library support. I think that will be the next step.

Tracking issue: rust-lang#116909

r? `@compiler-errors`
cc `@Nilstrieb`
`@rustbot` label +F-f16_and_f128
@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] rustc_infer test:false 92.838
   Compiling rustc_mir_build v0.0.0 (/checkout/compiler/rustc_mir_build)
[RUSTC-TIMING] rustc_codegen_ssa test:false 90.945
   Compiling rustc_lint v0.0.0 (/checkout/compiler/rustc_lint)
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
  local time: Sat Mar 16 00:01:55 UTC 2024
  local time: Sat Mar 16 00:01:55 UTC 2024
Session terminated, killing shell... ...killed.
##[error]The operation was canceled.
Cleaning up orphan processes

@bors
Copy link
Contributor

bors commented Mar 16, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 16, 2024
@tgross35
Copy link
Contributor Author

Clock drift failed, I don’t think that’s from the changes

@compiler-errors
Copy link
Member

@bors retry

@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 Mar 16, 2024
@bors
Copy link
Contributor

bors commented Mar 16, 2024

⌛ Testing commit 2098fec with merge c03ea3d...

@bors
Copy link
Contributor

bors commented Mar 16, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors,petrochenkov
Pushing c03ea3d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 16, 2024
@bors bors merged commit c03ea3d into rust-lang:master Mar 16, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 16, 2024
@tgross35 tgross35 deleted the f16-f128-step3-feature-gate branch March 16, 2024 04:07
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c03ea3d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 670.3s -> 670.952s (0.10%)
Artifact size: 311.50 MiB -> 311.63 MiB (0.04%)

fmease added a commit to fmease/rust that referenced this pull request Apr 10, 2024
…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
fmease added a commit to fmease/rust that referenced this pull request Apr 10, 2024
…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
fmease added a commit to fmease/rust that referenced this pull request Apr 10, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2024
…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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.