Skip to content

rustc_intrinsic: support functions without body #135031

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

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 2, 2025

We synthesize a HIR body loop {} but such bodyless intrinsics.

Most of the diff is due to turning ItemKind::Fn into a brace (named-field) enum variant, because it carries a bool-typed field now. This is to remember whether the function has a body. MIR building panics to avoid ever translating the fake loop {} body, and the intrinsic logic uses the lack of a body to implicitly mark that intrinsic as must-be-overridden.

I first tried actually having no body rather than generating the fake body, but there's a lot of code that assumes that all function items have HIR and MIR, so this didn't work very well. Then I noticed that even rustc_intrinsic_must_be_overridden intrinsics have MIR generated (they are filled with an Unreachable terminator) so I guess I am not the first to discover this. ;)

r? @oli-obk

@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. labels Jan 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2025

HIR ty lowering was modified

cc @fmease

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the intrinsics-without-body branch from 500a2b1 to 2356f2e Compare January 2, 2025 15:46
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2025

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the intrinsics-without-body branch from 2356f2e to 3411cd8 Compare January 2, 2025 15:58
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2025

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the intrinsics-without-body branch 2 times, most recently from ddd771e to 71572b6 Compare January 2, 2025 17:02
@RalfJung RalfJung force-pushed the intrinsics-without-body branch 4 times, most recently from 7863029 to 958a710 Compare January 2, 2025 20:09
@oli-obk
Copy link
Contributor

oli-obk commented Jan 3, 2025

Most of the diff is due to turning ItemKind::Fn into a tuple enum variant, because it carries a bool-typed field now.

I think you mean turning the tuple variant into a struct variant.

Also, annoying work, but I would prefer this change to be in a previous commit and then just adding the new field in a later commit.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 3, 2025 via email

@oli-obk
Copy link
Contributor

oli-obk commented Jan 4, 2025

I can review as is. The effort for me to review as is is likely less than reviewing after splitting and you doing the splitting. Just keep it in mind for future stuff.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 4, 2025

I can try git add -p, if that works it's hopefully not too bad.

I had no idea what I would have to do for this PR. I had to do a bunch of experimentation until something worked -- even for the future I wouldn't know how to have a nice git history for that without a lot of extra effort.

@RalfJung RalfJung force-pushed the intrinsics-without-body branch from 958a710 to 3cd3649 Compare January 4, 2025 10:42
@RalfJung
Copy link
Member Author

RalfJung commented Jan 4, 2025

Yeah that strategy wasn't too bad, took around 20 minutes.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 4, 2025

even for the future I wouldn't know how to have a nice git history for that without a lot of extra effort.

Whenever I encounter a situation where I want to change a tuple struct/variant to a struct/struct-variant or vice versa, I stash (well nowadays I jj new) and do that change first and then continue. It's very useful if you realize you didn't want that after all. To me it's less effort as I usually end up in such a situation while nothing builds anyway, so I can make the refactor build on its own. YMMV.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 4, 2025

Thanks, that made it reviewable on my phone xD

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 4, 2025

📌 Commit 3cd3649 has been approved by oli-obk

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 Jan 4, 2025
@bors
Copy link
Collaborator

bors commented Jan 4, 2025

⌛ Testing commit 3cd3649 with merge fd127a3...

@bors
Copy link
Collaborator

bors commented Jan 4, 2025

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing fd127a3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 4, 2025
@bors bors merged commit fd127a3 into rust-lang:master Jan 4, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 4, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fd127a3): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
0.1% [0.1%, 0.1%] 1
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.1% [0.1%, 0.1%] 1

Max RSS (memory usage)

Results (primary 2.2%, secondary 1.8%)

This 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.

mean range count
Regressions ❌
(primary)
2.2% [1.4%, 3.1%] 2
Regressions ❌
(secondary)
1.8% [1.8%, 1.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [1.4%, 3.1%] 2

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: 764.045s -> 763.089s (-0.13%)
Artifact size: 325.60 MiB -> 325.61 MiB (0.00%)

@RalfJung RalfJung deleted the intrinsics-without-body branch January 6, 2025 14:47
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 24, 2025
…ust_be_overridden, r=oli-obk

remove `#[rustc_intrinsic_must_be_overridde]`

In rust-lang#135031, we gained support for just leaving away the body. Now that the bootstrap compiler got bumped, stop using the old style and remove support for it.

r? `@oli-obk`

There are a few more mentions of this attribute in RA code that I didn't touch; Cc `@rust-lang/rust-analyzer`
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Feb 25, 2025
…ust_be_overridden, r=oli-obk

remove `#[rustc_intrinsic_must_be_overridde]`

In rust-lang#135031, we gained support for just leaving away the body. Now that the bootstrap compiler got bumped, stop using the old style and remove support for it.

r? `@oli-obk`

There are a few more mentions of this attribute in RA code that I didn't touch; Cc `@rust-lang/rust-analyzer`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2025
Rollup merge of rust-lang#137489 - RalfJung:no-more-rustc_intrinsic_must_be_overridden, r=oli-obk

remove `#[rustc_intrinsic_must_be_overridde]`

In rust-lang#135031, we gained support for just leaving away the body. Now that the bootstrap compiler got bumped, stop using the old style and remove support for it.

r? `@oli-obk`

There are a few more mentions of this attribute in RA code that I didn't touch; Cc `@rust-lang/rust-analyzer`
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Feb 26, 2025
…erridden, r=oli-obk

remove `#[rustc_intrinsic_must_be_overridde]`

In rust-lang/rust#135031, we gained support for just leaving away the body. Now that the bootstrap compiler got bumped, stop using the old style and remove support for it.

r? `@oli-obk`

There are a few more mentions of this attribute in RA code that I didn't touch; Cc `@rust-lang/rust-analyzer`
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
…ust_be_overridden, r=oli-obk

remove `#[rustc_intrinsic_must_be_overridde]`

In rust-lang#135031, we gained support for just leaving away the body. Now that the bootstrap compiler got bumped, stop using the old style and remove support for it.

r? `@oli-obk`

There are a few more mentions of this attribute in RA code that I didn't touch; Cc `@rust-lang/rust-analyzer`
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
…ust_be_overridden, r=oli-obk

remove `#[rustc_intrinsic_must_be_overridde]`

In rust-lang#135031, we gained support for just leaving away the body. Now that the bootstrap compiler got bumped, stop using the old style and remove support for it.

r? `@oli-obk`

There are a few more mentions of this attribute in RA code that I didn't touch; Cc `@rust-lang/rust-analyzer`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-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.

6 participants