Lower intrinsics calls: forget, size_of, unreachable, wrapping_* #79049
Conversation
This allows constant propagation to evaluate `size_of` and `wrapping_*`, and unreachable propagation to propagate a call to `unreachable`. The lowering is performed as a MIR optimization, rather than during MIR building to preserve the special status of intrinsics with respect to unsafety checks and promotion.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 7188f55140098182d6378946b518683eed3c6937 with merge 702dc8081c7c9f77501bbdb6bad0af62a43dc781... |
|
☀️ Try build successful - checks-actions |
|
Queued 702dc8081c7c9f77501bbdb6bad0af62a43dc781 with parent 30e49a9, future comparison URL. |
|
Finished benchmarking try commit (702dc8081c7c9f77501bbdb6bad0af62a43dc781): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
@bors r+ |
|
📌 Commit 7188f55140098182d6378946b518683eed3c6937 has been approved by |
bors
added
S-waiting-on-bors
| source_info: terminator.source_info, | ||
| kind: StatementKind::Assign(box ( | ||
| destination, | ||
| Rvalue::NullaryOp(NullOp::SizeOf, tp_ty), |
There was a problem hiding this comment.
Hmm... we could also remove this nullary op and replace it with Rvalue::Use(Operand::Constant(...)). This applies to all nullary const fn.
There was a problem hiding this comment.
We could also teach constant propagation to evalaute rust intrinsics more generally?
There was a problem hiding this comment.
This applies to all nullary const fn.
Not anymore, now that CTFE can allocate memory (which landed after this was written)... it only applies to most nullary intrinsics.
|
cc @rust-lang/wg-mir-opt |
|
⌛ Testing commit 7188f55140098182d6378946b518683eed3c6937 with merge e8db247bb5311146388688d8e52c896305c4f9f1... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Ah, the test output changes on |
tmiasko
force-pushed
the
lower-intrinsics
branch
from
7188f55
to
6903273
Compare
|
@bors r+ |
|
📌 Commit 6903273 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Interesting PR, thanks! Would be nice to Cc @rust-lang/miri for things involving intrinsics as this made some code in Miri (and the CTFE core engine) dead that used to implement those intrinsics. Also, I expected to see code being removed from the LLVM backend now that it does not have to handle these intrinsics any more; is there any reason this code is kept around? |
| @@ -390,6 +391,7 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { | |||
|
|
|||
| // The main optimizations that we do on MIR. | |||
| let optimizations: &[&dyn MirPass<'tcx>] = &[ | |||
| &lower_intrinsics::LowerIntrinsics, | |||
There was a problem hiding this comment.
Currently, the optimization is run even with mir-opt-level=0. This means codegen and CTFE can rely on these intrinsics not existing any more. Is that deliberate? For something called "lowering", it makes sense to be able to rely on it, but then it should be moved to run_post_borrowck_cleanup_passes IMO.
There was a problem hiding this comment.
It doesn't run at mir-opt-level=0. The default situation might also change if CTFE is to be run on unoptimized MIR. Always lowering them seems fine to me, modulo concerns already mentioned in the PR description.
There was a problem hiding this comment.
Are you sure? In #79989 I tried -Zmir-opt-level=0 and it did not make a difference...
There was a problem hiding this comment.
I see that the pass is only added in optimizations, but then CTFE should fail when the forget intrinsic is called in #79989, and yet I still see this trace:
│ │└┐rustc_mir::interpret::eval_context::frame std::mem::forget::<Vec<i32>>
│ │ ├─0ms INFO rustc_mir::interpret::step // executing bb0
│ │ ├─0ms INFO rustc_mir::interpret::step _0 = const ()
│ │ ├─0ms INFO rustc_mir::interpret::step return
│ │ ├─0ms INFO rustc_mir::interpret::eval_context popping stack frame (returning from function)
│ │┌┘rustc_mir::interpret::eval_context::frame std::mem::forget::<Vec<i32>>
Note the lack of anything happening in mem::forget. This was with RUSTC_LOG=rustc_mir::interpret=info rustc +196ca161be8363503d2d2445bab9840f0ce2bba4 /tmp/forget.rs -Zmir-opt-level=0, on this code:
fn main() {
const fn forget_arg_and_return_4(x: Vec<i32>) -> i32 {
std::mem::forget(x);
4
}
const FOUR_THE_HARD_WAY: i32 = forget_arg_and_return_4(Vec::new());
assert_eq!(FOUR_THE_HARD_WAY, 4);
}There was a problem hiding this comment.
Unless you are building std with -Zmir-opt-level=0 it is already too late for it to have an effect for non-local MIR.
There was a problem hiding this comment.
Oh, right... so we now have a barely tested code path here, namely the intrinsic implementations in CTFE and codegen, which is only hit when std is compiled with mit-opr-level=0. That seems potentially problematic.
remove intrinsic that is now implemented in the rustc side Since rust-lang/rust#80040, we can rely on the pass introduced in rust-lang/rust#79049 to lower away `forget`.
This allows constant propagation to evaluate
size_ofandwrapping_*,and unreachable propagation to propagate a call to
unreachable.The lowering is performed as a MIR optimization, rather than during MIR
building to preserve the special status of intrinsics with respect to
unsafety checks and promotion.
Currently enabled by default to determine the performance impact (no
significant impact expected). In practice only useful when combined with
inlining since intrinsics are rarely used directly (with exception of
unreachableanddiscriminant_valueused by built-in derive macros).Closes #32716.