Distinguish lowered-to-MIR intrinsics from handled-by-backend intrinsics #133690
Conversation
|
Could not assign reviewer from: |
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
the
S-waiting-on-review
|
Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @rust-lang/wg-const-eval |
rustbot
added
T-compiler
| /// large and difficult to optimize. | ||
| /// | ||
| /// The stabilized version of this intrinsic is [`Ord::cmp`]. | ||
| #[rustc_nounwind] |
There was a problem hiding this comment.
This is, of course, not actually a critical part of this PR, but it jumped out to me as I was updating attributes.
Let me know if you'd rather I did it in a different PR instead.
This comment has been minimized.
This comment has been minimized.
| #[rustc_intrinsic] | ||
| #[rustc_intrinsic_must_be_overridden] | ||
| #[cfg_attr(bootstrap, rustc_intrinsic_must_be_overridden)] | ||
| #[cfg_attr(not(bootstrap), rustc_intrinsic_lowers_to_mir)] |
There was a problem hiding this comment.
I think I would prefer not requiring any annotations for intrinsics that are actually intrinsics (i.e., implemented by the compiler). Why should the library care whether the compiler implements these via lowering to MIR or in the backend? All intrinsics really are is a way to add more primitive operations to MIR without having to extend the AST. For some operations we keep the "function call" representation all the way to the backend, for some we have a more direct MIR representation -- but that's a compiler-internal implementation detail.
There was a problem hiding this comment.
I think it would be nice for backends to be able to tell what they need to handle, though, and not write implementations of things that aren't needed. Not to mention that this way if you say it's handled in MIR, it actually checks it.
Being a compiler implementation detail seems fine, since intrinsics are all compiler-internal implementation details, really. And the distinction matters for things like stable MIR too, since it can ignore anything that's mir-lowered.
There was a problem hiding this comment.
intrinsics are all compiler-internal implementation details, really
They are all internal to this repo, but they are not internal to compiler/. Their names and types are shared with library/. But the way they get lowered is private to the compiler, and exposing that in the library seems odd to me.
For stable MIR these are function calls, I don't understand that part of your comment.
There was a problem hiding this comment.
I think it would be nice for backends to be able to tell what they need to handle, though, and not write implementations of things that aren't needed.
Do backend people read this file to figure that out? I would assume they run a test suite, which then tells them quite clearly what they need to implement. That's how I did it for Miri, anyway.
"Must be overridden" is misleading for things that a backend actually shouldn't override since they'll never see them.
scottmcm
force-pushed
the
intrinsic-lowers-to-mir
branch
from
4db6177 to
45dcd90
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
5 participants
"Must be overridden" is misleading for things that a backend actually shouldn't override since they'll never see them.
r? oli-obk