Repository navigation
UB Check blocks MIR inlining of Vec::deref #123174
Description
Activity
- addedC-bugCategory: This is a bug.Category: This is a bug.regression-untriagedUntriaged performance or correctness regression.Untriaged performance or correctness regression.
on Mar 28, 2024 - addedI-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}Issue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}needs-triageThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triagingThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triagingregression-from-stable-to-betaPerformance or correctness regression from stable to beta.Performance or correctness regression from stable to beta.
on Mar 28, 2024 #121662 seems highly relevant, in that it introduced the monomorphization time branch IIUC
#122975 may help, can you try again once that lands?
That does look likely to be relevant, at least to the open-coded version; that the version using
<Vec<_> as Deref>::derefisn't inlining likely means that it still won't and thus won't be able to seeUbCheckstransition from unknown to constant via inlining.On the other hand, it somewhat feels like
assert_unsafe_precondition!should be checkingcfg!(ub_checks)"one level" up (e.g. fromcore::sliceintoalloc::vec) instead of "all the way" up (i.e. out of the sysroot crates); perhaps, in analogy to how overflow checks are handled, it's wanting for a per-function#[rustc_inherit_ub_checks]instead of a crate-wide configuration? (Although even then,unsafe fnshould probably default to inheriting UB checks configuration.)WG-prioritization assigning priority (Zulip discussion).
@rustbot label -I-prioritize +P-medium
- addedP-mediumMedium priorityMedium priorityand removedI-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}Issue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
on Mar 28, 2024 - addedA-mir-optArea: MIR optimizationsArea: MIR optimizationsand removedC-bugCategory: This is a bug.Category: This is a bug.needs-triageThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triagingThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triaging
on Mar 28, 2024 I am not sure that PR fixes this. The replacement of the NullOp would occur after inlining, so we need the function to be a good inlining candidate in the first place in order to have its UbCheck removed. Resolving that was why I wrote #121767, to remove the huge penalty from the call.
We could also special-case the check calls, they already have a special inliner attribute so what's a bit more magic 🙃
Reacted by Crystal DurhamI tried this out with my reworked costs in #123179, but it still doesn't inline by default -- it needs the threshold to be raised to
-Z inline-mir-hint-threshold=110to get inlined with those new costs.Seems like it's right on the edge. On nightly (2024-03-27) it took
-Z inline-mir-hint-threshold=105to inline https://rust.godbolt.org/z/7bK7n4d66, so it's only one instruction away.It was so close to the threshold that a touch of simplification in libcore got it inlining again for you.
PR up: #123190
Reacted by Mateusz Mikuła- linked a pull request that will close this issue[mir-inlining] Count `&T` ↔ `*const T` conversions as free #123843
on Apr 12, 2024 ...instead of that previous one, changed to a new more holistic PR that also fixes this #123840
- added a commit that references this issue
on Apr 21, 2024 - added 2 commits that reference this issue
on Apr 21, 2024 - added a commit that references this issue
on Apr 21, 2024
Code
I tried this code: [playground]
I expected to see this happen: in release mode with default MIR optimization, both
closedandopenwould generate equivalent optimized MIR — an inlined conversion from&Vec<u8>to&[u8](deconstruct theVecinto ptr and len then build the&[_]).Instead, this happened: On 1.77.0, the inlining happens as expected. On 1.78.0-beta.3 (2024-03-27 4147533),
<Vec<u8> as Deref>::deref(vec_derefforopen) is called outline. Impactfully, the call is additionally not known to never unwind (-> [return: …, unwind continue]in MIR), which likely impedes optimization around the call.vec_derefMIR on stablevec_derefMIR on betaOn stable,
vec_deref's optimized MIR is a single straightline basic block. On beta, it contains aswitchIntonUbCheck(LanguageUb)guarding a conditional call tostd::slice::from_raw_parts::precondition_check, resulting in four basic blocks including an MIR call operation. AIUI both of these metrics (block/call count) directly contribute to the MIR inlining heuristics.stable
slice::from_raw_partsbeta
slice::from_raw_partsVersion it worked on
It most recently worked on: Rust Stable 1.77.0
Version with regression
It isn't working on: Rust Beta 1.78.0-beta.3 (2024-03-27 4147533)
Meta
Found on irlo while looking at inlining behavior of
ToString.@rustbot modify labels: +regression-from-stable-to-beta
(didn't do
-regression-untriagedsince I didn't try to narrow it any further than playground stable/beta, but don't know the convention here)cc @saethlin (IIRC you've been working on
assert_unsafe_precondition!s and otherprecondition_checks)