Add a scheme for moving away from `extern "rust-intrinsic"` entirely #120675

oli-obk · 2024-02-05T12:26:45Z

All rust-intrinsics can become free functions now, either with a fallback body, or with a dummy body and an attribute, requiring backends to actually implement the intrinsic.

This PR demonstrates the dummy-body scheme with the vtable_size intrinsic.

cc #63585

follow-up to #120500

MCP at rust-lang/compiler-team#720

rustbot · 2024-02-05T12:26:54Z

r? @pnkfelix

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2024-02-05T12:26:56Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

tests/ui/intrinsics/not-overridden.stderr

oli-obk · 2024-02-05T12:29:07Z

compiler/rustc_monomorphize/src/collector.rs

+    if tcx.has_attr(def_id, sym::rustc_intrinsic_must_be_overridden) {
+        // These are implemented by backends directly and have no meaningful body.
+        return false;
+    }


This is what causes linker errors (prevents shipping bodies of such functions) if the backend accidentally tries to actually use the body.

oli-obk · 2024-02-05T12:30:11Z

compiler/rustc_mir_transform/src/lib.rs

+    if let Some(attr) = tcx.get_attr(did, sym::rustc_intrinsic_must_be_overridden) {
+        span_bug!(
+            attr.span,
+            "this intrinsic must be overridden by the codegen backend, it has no meaningful body",
+        )
+    }


Sanity check preventing actually ending up with MIR for these functions. Can't actually get hit due to the mono collector bailout

src/doc/unstable-book/src/language-features/intrinsics.md

pnkfelix · 2024-02-05T15:32:41Z

src/doc/unstable-book/src/language-features/intrinsics.md

@@ -24,6 +24,13 @@ fn foo() -> u32 {
 }
 ```

+## Intrinsics lowered to MIR instructions
+
+Various intrinsics have native MIR operations that they correspond to. Instead of requiring


The other two sections (Intrinsics {with, without} fallback logic) show how each respective kind of intrinsic is declared. Can you give a hint here as to how an Intrinsic lowered-to-MIR-instructions is declared?

pnkfelix · 2024-02-05T15:35:36Z

compiler/rustc_hir_analysis/src/check/check.rs

-        DefKind::Fn => {} // entirely within check_item_body
+        DefKind::Fn => {
+            if let Some(name) = tcx.intrinsic(def_id) {
+                intrinsic::check_intrinsic_type(


out of curiosity, do you know if we are testing the check_intrinsic_type logic anywhere in our unit tests? (E.g. via a #![no_core] test that adds its own intrinsics?)

okay I see b9d8f00 so there is at least some testing

compiler/rustc_middle/src/ty/util.rs

src/doc/unstable-book/src/language-features/intrinsics.md

compiler/rustc_codegen_ssa/src/mir/block.rs

pnkfelix · 2024-02-05T16:12:46Z

@oli-obk I think @RalfJung is right; go make an MCP and link this to it. Hopefully it shouldn't be too hard to put together.

pnkfelix · 2024-02-05T16:13:59Z

@rustbot label: +S-waiting-on-author -S-waiting-on-review

pnkfelix · 2024-02-05T16:17:13Z

Hmm. I need to read the descriptions more carefully, I missed the bit saying that only the last two commits were new. (And I didn't check PR #120500, so I missed the detall that those earlier commits are being landed independently.)

RalfJung · 2024-02-05T17:30:17Z

For some intrinsics having a fallback body makes no sense, as there is either no reasonable default or it's something absolutely compiler internal (e.g. size_of). In that case, one can add another #[rustc_intrinsic_must_be_overridden] to the function. This causes the function body to not get monomorphized and sent to the backend. The implementation has various assertions that prevent screwing this up. So if a backend forgets to implement, at best they get an assertion explaining what's going on, at worst they get a linker error.

Since the interpreter doesn't go through monomorphization, let's make sure it also has such an assertion. :)

bors · 2024-03-04T19:32:36Z

📌 Commit c04f0ca has been approved by pnkfelix

It is now in the queue for this repository.

bors · 2024-03-05T00:13:04Z

⌛ Testing commit c04f0ca with merge 2eeff46...

coolreader18 · 2024-03-05T02:52:16Z

Maybe I'm missing something here, but is there a reason not to just allow #[rustc_intrinsic_must_be_overridden] functions to have no body at all? Free functions having bodies is a semantic requirement, not a syntactic one, so I'd think that would parse fine.

bors · 2024-03-05T02:53:53Z

☀️ Test successful - checks-actions
Approved by: pnkfelix
Pushing 2eeff46 to master...

rust-timer · 2024-03-05T04:09:07Z

Finished benchmarking commit (2eeff46): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.2%, 1.0%]	93
Regressions ❌ (secondary)	0.8%	[0.2%, 2.4%]	28
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.2%, 1.0%]	93

Max RSS (memory usage)

Results

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)	3.9%	[0.8%, 16.7%]	7
Regressions ❌ (secondary)	7.9%	[6.2%, 11.2%]	3
Improvements ✅ (primary)	-2.7%	[-3.9%, -1.0%]	9
Improvements ✅ (secondary)	-2.7%	[-4.2%, -1.5%]	31
All ❌✅ (primary)	0.2%	[-3.9%, 16.7%]	16

Cycles

Results

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)	-	-	0
Regressions ❌ (secondary)	3.1%	[3.0%, 3.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 644.33s -> 644.326s (-0.00%)
Artifact size: 174.98 MiB -> 174.98 MiB (0.00%)

Mark-Simulacrum · 2024-03-05T16:57:25Z

Regression is being addressed in #122010.

Add a scheme for moving away from `extern "rust-intrinsic"` entirely All `rust-intrinsic`s can become free functions now, either with a fallback body, or with a dummy body and an attribute, requiring backends to actually implement the intrinsic. This PR demonstrates the dummy-body scheme with the `vtable_size` intrinsic. cc rust-lang#63585 follow-up to rust-lang#120500 MCP at rust-lang/compiler-team#720

Avoid invoking the `intrinsic` query for DefKinds other than `Fn` or `AssocFn` fixes the perf regression from rust-lang#120675 by only invoking (and thus inserting into the dep graph) the `intrinsic` query if the `DefKind` matches items that can actually be intrinsics

Avoid invoking the `intrinsic` query for DefKinds other than `Fn` or `AssocFn` fixes the perf regression from rust-lang/rust#120675 by only invoking (and thus inserting into the dep graph) the `intrinsic` query if the `DefKind` matches items that can actually be intrinsics

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

Avoid invoking the `intrinsic` query for DefKinds other than `Fn` or `AssocFn` fixes the perf regression from rust-lang/rust#120675 by only invoking (and thus inserting into the dep graph) the `intrinsic` query if the `DefKind` matches items that can actually be intrinsics

rust-lang#120675 introduced a new mechanism to declare intrinsics which will potentially replace the rust-intrinsic ABI. The new mechanism introduces a placeholder body and mark the intrinsic with #[rustc_intrinsic_must_be_overridden]. In practice, this means that backends should not generate code for the placeholder, and shim the intrinsic. The new annotation is an internal compiler implementation, and it doesn't need to be exposed to StableMIR users. In this PR, intrinsics marked with `rustc_intrinsic_must_be_overridden` are handled the same way as intrinsics that do not have a body.

…li-obk Unify intrinsics body handling in StableMIR rust-lang#120675 introduced a new mechanism to declare intrinsics which will potentially replace the rust-intrinsic ABI. The new mechanism introduces a placeholder body and mark the intrinsic with `#[rustc_intrinsic_must_be_overridden]`. In practice, this means that a backend should not generate code for the placeholder, and shim the intrinsic. The new annotation is an internal compiler implementation, and it doesn't need to be exposed to StableMIR users. In this PR, we unify the interface for intrinsics marked with `rustc_intrinsic_must_be_overridden` and intrinsics that do not have a body. Fixes rust-lang/project-stable-mir#79 r? `@oli-obk` cc: `@momvart`

…li-obk Unify intrinsics body handling in StableMIR rust-lang#120675 introduced a new mechanism to declare intrinsics which will potentially replace the rust-intrinsic ABI. The new mechanism introduces a placeholder body and mark the intrinsic with `#[rustc_intrinsic_must_be_overridden]`. In practice, this means that a backend should not generate code for the placeholder, and shim the intrinsic. The new annotation is an internal compiler implementation, and it doesn't need to be exposed to StableMIR users. In this PR, we unify the interface for intrinsics marked with `rustc_intrinsic_must_be_overridden` and intrinsics that do not have a body. Fixes rust-lang/project-stable-mir#79 r? ``@oli-obk`` cc: ``@momvart``

Rollup merge of rust-lang#126361 - celinval:issue-0079-intrinsic, r=oli-obk Unify intrinsics body handling in StableMIR rust-lang#120675 introduced a new mechanism to declare intrinsics which will potentially replace the rust-intrinsic ABI. The new mechanism introduces a placeholder body and mark the intrinsic with `#[rustc_intrinsic_must_be_overridden]`. In practice, this means that a backend should not generate code for the placeholder, and shim the intrinsic. The new annotation is an internal compiler implementation, and it doesn't need to be exposed to StableMIR users. In this PR, we unify the interface for intrinsics marked with `rustc_intrinsic_must_be_overridden` and intrinsics that do not have a body. Fixes rust-lang/project-stable-mir#79 r? ``@oli-obk`` cc: ``@momvart``

Unify intrinsics body handling in StableMIR rust-lang/rust#120675 introduced a new mechanism to declare intrinsics which will potentially replace the rust-intrinsic ABI. The new mechanism introduces a placeholder body and mark the intrinsic with `#[rustc_intrinsic_must_be_overridden]`. In practice, this means that a backend should not generate code for the placeholder, and shim the intrinsic. The new annotation is an internal compiler implementation, and it doesn't need to be exposed to StableMIR users. In this PR, we unify the interface for intrinsics marked with `rustc_intrinsic_must_be_overridden` and intrinsics that do not have a body. Fixes rust-lang/project-stable-mir#79 r? ``@oli-obk`` cc: ``@momvart``

rustbot assigned pnkfelix Feb 5, 2024

oli-obk commented Feb 5, 2024

View reviewed changes

tests/ui/intrinsics/not-overridden.stderr Outdated Show resolved Hide resolved

oli-obk commented Feb 5, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

oli-obk added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Feb 5, 2024

This comment has been minimized.

Sign in to view

oli-obk force-pushed the intrinsics3.0 branch from d587e44 to 95d7abf Compare February 5, 2024 15:01

This comment has been minimized.

Sign in to view

pnkfelix reviewed Feb 5, 2024

View reviewed changes

src/doc/unstable-book/src/language-features/intrinsics.md Outdated Show resolved Hide resolved

pnkfelix reviewed Feb 5, 2024

View reviewed changes

compiler/rustc_middle/src/ty/util.rs Outdated Show resolved Hide resolved

RalfJung reviewed Feb 5, 2024

View reviewed changes

src/doc/unstable-book/src/language-features/intrinsics.md Show resolved Hide resolved

pnkfelix reviewed Feb 5, 2024

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/block.rs Outdated Show resolved Hide resolved

pnkfelix reviewed Feb 5, 2024

View reviewed changes

compiler/rustc_codegen_ssa/src/mir/block.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2024

oli-obk mentioned this pull request Feb 5, 2024

Remove fake rust-intrinsic ABI rust-lang/compiler-team#720

Closed

3 tasks

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2024

bors merged commit 2eeff46 into rust-lang:master Mar 5, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Mar 5, 2024

oli-obk deleted the intrinsics3.0 branch March 5, 2024 04:21

oli-obk mentioned this pull request Mar 5, 2024

Avoid invoking the intrinsic query for DefKinds other than Fn or AssocFn #122010

Merged

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Mar 5, 2024

zhassan-aws mentioned this pull request Mar 11, 2024

Upgrade toolchain to 2024-03-11 model-checking/kani#3071

Merged

celinval mentioned this pull request May 23, 2024

Proper handling of intrinsics with #[rustc_intrinsic_must_be_overridden] rust-lang/project-stable-mir#79

Open

celinval mentioned this pull request Jun 12, 2024

Unify intrinsics body handling in StableMIR #126361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a scheme for moving away from `extern "rust-intrinsic"` entirely #120675

Add a scheme for moving away from `extern "rust-intrinsic"` entirely #120675

oli-obk commented Feb 5, 2024 •

edited

Loading

rustbot commented Feb 5, 2024

rustbot commented Feb 5, 2024

oli-obk Feb 5, 2024 •

edited

Loading

oli-obk Feb 5, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pnkfelix Feb 5, 2024

pnkfelix Feb 5, 2024

pnkfelix Feb 5, 2024

pnkfelix commented Feb 5, 2024 •

edited

Loading

pnkfelix commented Feb 5, 2024

pnkfelix commented Feb 5, 2024

RalfJung commented Feb 5, 2024 •

edited

Loading

bors commented Mar 4, 2024

bors commented Mar 5, 2024

coolreader18 commented Mar 5, 2024

bors commented Mar 5, 2024

rust-timer commented Mar 5, 2024

Mark-Simulacrum commented Mar 5, 2024

Add a scheme for moving away from extern "rust-intrinsic" entirely #120675

Add a scheme for moving away from extern "rust-intrinsic" entirely #120675

Conversation

oli-obk commented Feb 5, 2024 • edited Loading

rustbot commented Feb 5, 2024

rustbot commented Feb 5, 2024

oli-obk Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Feb 5, 2024 • edited Loading

pnkfelix commented Feb 5, 2024

pnkfelix commented Feb 5, 2024

RalfJung commented Feb 5, 2024 • edited Loading

bors commented Mar 4, 2024

bors commented Mar 5, 2024

coolreader18 commented Mar 5, 2024

bors commented Mar 5, 2024

rust-timer commented Mar 5, 2024

Overall result: ❌ regressions - ACTION NEEDED

Mark-Simulacrum commented Mar 5, 2024

Add a scheme for moving away from `extern "rust-intrinsic"` entirely #120675

Add a scheme for moving away from `extern "rust-intrinsic"` entirely #120675

oli-obk commented Feb 5, 2024 •

edited

Loading

oli-obk Feb 5, 2024 •

edited

Loading

pnkfelix commented Feb 5, 2024 •

edited

Loading

RalfJung commented Feb 5, 2024 •

edited

Loading