Uh oh!
There was an error while loading. Please reload this page.
Implement intrinsics with fallback bodies - #120500
Merged
Merged
Conversation
rustbot
commented
Jan 30, 2024
Collaborator
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
commented
Jan 30, 2024
Collaborator
Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
bjorn3
reviewed
Jan 30, 2024
| /// Whether the function is an intrinsic | ||
| query is_intrinsic(def_id: DefId) -> bool { | ||
| desc { |tcx| "checking whether `{}` is an intrinsic", tcx.def_path_str(def_id) } | ||
| query intrinsic(def_id: DefId) -> Option<Symbol> { |
Member
There was a problem hiding this comment.
Would cg_clif need to use this too? It currently matches on the IntrinsicDef and uses tcx.item_name() when it is an intrinsic.
ContributorAuthor
There was a problem hiding this comment.
IntrinsicDef is only created if intrinsic returns Some. Otherwise you get a normal definition. That's why I needed to query the backend. I created this design because I think it is more robust than letting the IntrinsicDef go all the way to backends and have them figure out how to call the fallback body again.
Basically if the backend doesn't say it has an impl for a rustc_intrinsic intrinsic, then the intrinsic becomes just a normal function from the perspective of the entire compiler
Member
There was a problem hiding this comment.
The problem with the current design as far as I can tell is that the return value of intrinsic is hard coded in the crate metadata for libcore based on the codegen backend with which libcore is compiled. This would break linking crates compiled by different codegen backends together.
ContributorAuthor
There was a problem hiding this comment.
Hmm... I guess I can make the backend check happen outside the query
ContributorAuthor
There was a problem hiding this comment.
This is not relevant anymore. They are always intrinsics now, and the backends can choose to call the default body instead.
bjorn3
reviewed
Jan 30, 2024
| .name, | ||
| ) | ||
| .name; | ||
| tcx.intrinsics_implemented_by_backend(()).contains(&name).then_some(name) |
Member
There was a problem hiding this comment.
This seems like it is bound to go out of sync with what the backend actually supports. Would it be possible to instead obtain the fallback MIR body inside the codegen backend when the backend determines that there is no implementation? Also there are various places where is_intrinsic doesn't just affect codegen for the local crate. For example the lower intrinsics MIR pass (which uses is_intrinsic) affects the MIR bodies recorded in the crate metadata, const eval will use is_intrinsic to skip counting any execution cost for intrinsics. The value of is_intrinsic is also recorded in the crate metadata and thus only the codegen backend with which libcore is compiled effectively determines if is_intrinsic would return true. This completely breaks when mixing codegen backends, like mixing cg_clif with a cg_llvm sysroot as using the rustc-codegen-cranelift-preview component would do.
ContributorAuthor
There was a problem hiding this comment.
Hmm... I wanted to avoid that, but I agree it's annoying that they go out of sync.
I'll think about how to make backends able to call the default body.
Member
There was a problem hiding this comment.
Being able to call the default body would be really nice -- I was thinking about swap the other day, where being able to have a looping version as the default but then override it with something smarter sometimes in the codegen backend would be particularly nice if the codegen backend didn't also need to emit the loop version.
(That said it could always be approximated by having the intrinsic return a bool that says whether it did anything, for example, and thus getting the library fallback that way if it's particularly needed for some intrinsics.)
ContributorAuthor
There was a problem hiding this comment.
Done. The backends need to implement this logic themselves every time (I implemented it for cranelift), but that indeed seems like the best solution.
WaffleLapkin
approved these changes
Jan 30, 2024
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
scottmcm
commented
Jan 30, 2024
Member
Thanks for working on this, Oli! I'm really excited 🎉 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
commented
Jan 31, 2024
Collaborator
Some changes occurred in src/librustdoc/clean/types.rs cc @camelid |
This comment has been minimized.
This comment has been minimized.
oli-obk
commented
Jan 31, 2024
ContributorAuthor
Should finally be ready. We don't need miri support from the get go, so I'd rather do it in a separate PR. |
bjorn3
reviewed
Feb 1, 2024
| fx.tcx | ||
| .dcx() | ||
| .span_fatal(source_info.span, format!("unsupported intrinsic {}", intrinsic)); | ||
| } |
Member
There was a problem hiding this comment.
What will the error message be for intrinsics that are neither implemented by cg_clif nor have a fallback body?
bjorn3
commented
Feb 1, 2024
Member
cg_clif changes LGTM |
This comment has been minimized.
This comment has been minimized.
WaffleLapkin
approved these changes
Feb 2, 2024
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rustbot
commented
Feb 2, 2024
Collaborator
The Miri subtree was changed cc @rust-lang/miri |
oli-obk
commented
Feb 2, 2024
ContributorAuthor
@bors r=WaffleLapkin |
bors
commented
Feb 2, 2024
Collaborator
pnkfelix
commented
Feb 14, 2024
Contributor
The default log presentation was not expanding for me; below is what I think was causing the failure according to what I saw from skimming the raw log itself. Snippet of raw log output with ui test failures |
oli-obk
commented
Feb 14, 2024
ContributorAuthor
That's an error from a different PR: #120847 (see last commit) |
oli-obk added a commit
to oli-obk/rust
that referenced
this pull request
Feb 15, 2024
Implement intrinsics with fallback bodies fixesrust-lang#93145 (though we can port many more intrinsics) cc rust-lang#63585 The way this works is that the backend logic for generating custom code for intrinsics has been made fallible. The only failure path is "this intrinsic is unknown". The `Instance` (that was `InstanceDef::Intrinsic`) then gets converted to `InstanceDef::Item`, which represents the fallback body. A regular function call to that body is then codegenned. This is currently implemented for * codegen_ssa (so llvm and gcc) * codegen_cranelift other backends will need to adjust, but they can just keep doing what they were doing if they prefer (though adding new intrinsics to the compiler will then require them to implement them, instead of getting the fallback body). cc `@scottmcm` `@WaffleLapkin` ### todo * [ ] miri support * [x] default intrinsic name to name of function instead of requiring it to be specified in attribute * [x] make sure that the bodies are always available (must be collected for metadata)
bors
commented
Feb 16, 2024
Collaborator
bors
commented
Feb 16, 2024
Collaborator
☀️ Test successful - checks-actions |
This was referenced Feb 16, 2024
Merged
Merged
Closed
rust-timer
commented
Feb 16, 2024
Collaborator
Finished benchmarking commit (dfa88b3): 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 637.782s -> 637.11s (-0.11%) |
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 22, 2024
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
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 22, 2024
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
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 23, 2024
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
celinval added a commit
to model-checking/kani
that referenced
this pull request
Feb 26, 2024
Upgrade toolchain to 2024-02-17. Relevant PR: - rust-lang/rust#120500 - rust-lang/rust#100603Fixes#87Fixes#3034Fixes#3037
flip1995 pushed a commit
to flip1995/rust
that referenced
this pull request
Feb 26, 2024
Implement intrinsics with fallback bodies fixesrust-lang#93145 (though we can port many more intrinsics) cc rust-lang#63585 The way this works is that the backend logic for generating custom code for intrinsics has been made fallible. The only failure path is "this intrinsic is unknown". The `Instance` (that was `InstanceDef::Intrinsic`) then gets converted to `InstanceDef::Item`, which represents the fallback body. A regular function call to that body is then codegenned. This is currently implemented for * codegen_ssa (so llvm and gcc) * codegen_cranelift other backends will need to adjust, but they can just keep doing what they were doing if they prefer (though adding new intrinsics to the compiler will then require them to implement them, instead of getting the fallback body). cc `@scottmcm` `@WaffleLapkin` ### todo * [ ] miri support * [x] default intrinsic name to name of function instead of requiring it to be specified in attribute * [x] make sure that the bodies are always available (must be collected for metadata)
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 4, 2024
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
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 5, 2024
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
flip1995 pushed a commit
to flip1995/rust
that referenced
this pull request
Mar 7, 2024
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
Contributor
Sorry, I just now realized that |
RalfJung
commented
Apr 28, 2024
via email
Member
Note that is_val_statically_known and const_eval_select are entirely unrelated. (The first is about const propagation, the second about Const eval / CTFE. These two concepts have very little to do with each other.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes#93145 (though we can port many more intrinsics)
cc #63585
The way this works is that the backend logic for generating custom code for intrinsics has been made fallible. The only failure path is "this intrinsic is unknown". The
Instance(that wasInstanceDef::Intrinsic) then gets converted toInstanceDef::Item, which represents the fallback body. A regular function call to that body is then codegenned. This is currently implemented forother backends will need to adjust, but they can just keep doing what they were doing if they prefer (though adding new intrinsics to the compiler will then require them to implement them, instead of getting the fallback body).
cc @scottmcm@WaffleLapkin
todo