Only check the DefId for the recursion check in MIR inliner.

[cjgillot]

The current history check compares Instances, so it cannot detect cases of polymorphic recursion where Substs change.
This PR makes it so we only compare DefIds, ignoring any change in Substs.

According to #100522 (comment), in practice only very few inlining decisions change.

Fixes #100476

[rust-highfive]

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[tmiasko]

Waiting until a depth limit is hit is not enough to avoid the issue in a variations of the original code, e.g., with two self-recursive calls instead of just one.

Maybe keep a history with def ids only? That should be quite robust. In fact there used to be a self-recursion check based on def id only, but looks like it is gone now?. While in the principle there are cases with recursive def ids which we would be good candidates for inlining, they are relatively uncommon in practice. While bootstraping rustc, a recursion check that uses def ids only differs from the one that also includes substitutions only 26 times.

[lcnr]

r? @tmiasko looks like you have thoughts on how this should be handled

[cjgillot]

Waiting until a depth limit is hit is not enough to avoid the issue in a variations of the original code, e.g., with two self-recursive calls instead of just one.

Why? With two self-recursive calls, each inlined call would increase the length of history, wouldn't it? It works as expected on:

fn inner<T>() { inner2<T::Next>() }
fn inner2<T>() { inner<T::Next>() }

Maybe keep a history with def ids only? That should be quite robust. In fact there used to be a self-recursion check based on def id only, but looks like it is gone now?. While in the principle there are cases with recursive def ids which we would be good candidates for inlining, they are relatively uncommon in practice. While bootstraping rustc, a recursion check that uses def ids only differs from the one that also includes substitutions only 26 times.

I could only find you PR which introduced the check based on Instance 😉. I guess we can.
Do you have the list of those 26 times? This would tell which case we are regressing.

[tmiasko]

With two self-recursive calls, each inlined call would increase the length of history, wouldn't it?

I had in mind a case with two calls, where each call site has an independent history, growing in both breadth and depth:

        #[inline(always)]
        fn inner<const N: usize, T: Tr>() {
            inner::<N, T::Next>();
            inner::<N, T::Next>();
        }

I could only find you PR which introduced the check based on Instance 😉.

The check based on def id was removed in 114c928 (it prevented inlining of functions with direct self-recursion).

Do you have the list of those 26 times? This would tell which case we are regressing.

Differences: instance that is being optimized, callee

MirSource { instance: Item(WithOptConstParam { did: DefId(0:46730 ~ core[ef75]::iter::adapters::peekable::{impl#8}::clone), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(0:9068 ~ core[ef75]::option::{impl#5}::clone), const_param_did: None }), substs: [<I as iter::traits::iterator::Iterator>::Item] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:93 ~ rustc_demangle[b6dd]::v0::{impl#3}::try_parse_str_chars::{closure#2}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [[&u8; 2], rustc_std_workspace_core::slice::ChunksExact<u8>, [closure@~/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-demangle-0.1.21/src/v0.rs:303:18: 303:25]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:94 ~ rustc_demangle[b6dd]::v0::{impl#3}::try_parse_str_chars::{closure#2}::{closure#0}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [[&u8; 2], rustc_std_workspace_core::slice::ChunksExact<u8>, [closure@~/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-demangle-0.1.21/src/v0.rs:303:18: 303:25]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:46730 ~ core[ef75]::iter::adapters::peekable::{impl#8}::clone), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(0:9068 ~ core[ef75]::option::{impl#5}::clone), const_param_did: None }), substs: [<I as iter::traits::iterator::Iterator>::Item] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:93 ~ rustc_demangle[b6dd]::v0::{impl#3}::try_parse_str_chars::{closure#2}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [[&u8; 2], rustc_std_workspace_core::slice::ChunksExact<u8>, [closure@~/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-demangle-0.1.21/src/v0.rs:303:18: 303:25]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:94 ~ rustc_demangle[b6dd]::v0::{impl#3}::try_parse_str_chars::{closure#2}::{closure#0}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [[&u8; 2], rustc_std_workspace_core::slice::ChunksExact<u8>, [closure@~/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-demangle-0.1.21/src/v0.rs:303:18: 303:25]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:93 ~ rustc_demangle[7ef0]::v0::{impl#3}::try_parse_str_chars::{closure#2}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(1:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [[&u8; 2], core::slice::ChunksExact<u8>, [closure@~/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-demangle-0.1.21/src/v0.rs:303:18: 303:25]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:94 ~ rustc_demangle[7ef0]::v0::{impl#3}::try_parse_str_chars::{closure#2}::{closure#0}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(1:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [[&u8; 2], core::slice::ChunksExact<u8>, [closure@~/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-demangle-0.1.21/src/v0.rs:303:18: 303:25]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:344 ~ cc[5fa3]::{impl#9}::clone), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:9068 ~ core[ef75]::option::{impl#5}::clone), const_param_did: None }), substs: [std::string::String] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:219 ~ cc[5fa3]::{impl#6}::get_cpp_link_stdlib), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:9068 ~ core[ef75]::option::{impl#5}::clone), const_param_did: None }), substs: [std::string::String] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:398 ~ tracing_core[54ca]::field::{impl#35}::field::{closure#0}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:3041 ~ core[ef75]::cmp::impls::{impl#9}::eq), const_param_did: None }), substs: [ReErased, ReErased, str, str] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:3564 ~ rustc_middle[f071]::mir::pretty::write_allocations::{closure#0}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:7008 ~ core[ef75]::iter::adapters::map::{impl#3}::next_back), const_param_did: None }), substs: [&mir::interpret::AllocId, std::slice::Iter<(rustc_target::abi::Size, mir::interpret::AllocId)>, [closure@rustc_data_structures::sorted_map::SortedMap<rustc_target::abi::Size, mir::interpret::AllocId>::values::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:7210 ~ rustc_middle[f071]::ty::layout::{impl#3}::layout_of_uncached::{closure#16}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [usize, std::ops::Range<usize>, [closure@rustc_target::abi::FieldsShape::index_by_increasing_offset::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:7190 ~ rustc_middle[f071]::ty::layout::{impl#3}::layout_of_uncached), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [(rustc_target::abi::VariantIdx, &ty::VariantDef), std::iter::Enumerate<std::slice::Iter<ty::VariantDef>>, [closure@rustc_index::vec::IndexVec<rustc_target::abi::VariantIdx, ty::VariantDef>::iter_enumerated::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:275 ~ rustc_mir_dataflow[2f90]::elaborate_drops::{impl#2}::open_drop_for_multivariant), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [(rustc_target::abi::VariantIdx, &rustc_middle::ty::VariantDef), std::iter::Enumerate<std::slice::Iter<rustc_middle::ty::VariantDef>>, [closure@rustc_index::vec::IndexVec<rustc_target::abi::VariantIdx, rustc_middle::ty::VariantDef>::iter_enumerated::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:1669 ~ rustc_metadata[11fd]::rmeta::encoder::{impl#11}::encode_def_path_table), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [(rustc_span::def_id::DefIndex, &rustc_hir::definitions::DefKey), std::iter::Enumerate<std::slice::Iter<rustc_hir::definitions::DefKey>>, [closure@rustc_index::vec::IndexVec<rustc_span::def_id::DefIndex, rustc_hir::definitions::DefKey>::iter_enumerated::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:1691 ~ rustc_metadata[11fd]::rmeta::encoder::{impl#12}::encode_def_ids), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [rustc_span::def_id::DefIndex, std::ops::Range<usize>, [closure@rustc_index::vec::IndexVec<rustc_span::def_id::DefIndex, rustc_span::def_id::DefPathHash>::indices::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:2314 ~ rustc_infer[52fb]::infer::error_reporting::{impl#0}::cmp::{closure#0}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:3041 ~ core[ef75]::cmp::impls::{impl#9}::eq), const_param_did: None }), substs: [ReErased, ReErased, rustc_middle::ty::Ty, rustc_middle::ty::Ty] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:2317 ~ rustc_infer[52fb]::infer::error_reporting::{impl#0}::cmp::{closure#1}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:3041 ~ core[ef75]::cmp::impls::{impl#9}::eq), const_param_did: None }), substs: [ReErased, ReErased, str, str] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:1302 ~ rustc_resolve[0ff7]::diagnostics::show_candidates::{closure#8}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:3041 ~ core[ef75]::cmp::impls::{impl#9}::eq), const_param_did: None }), substs: [ReErased, ReErased, str, str] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:135 ~ rustc_transmute[9aba]::layout::tree::rustc::{impl#3}::from_ty), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [(rustc_target::abi::VariantIdx, &rustc_middle::ty::VariantDef), std::iter::Enumerate<std::slice::Iter<rustc_middle::ty::VariantDef>>, [closure@rustc_index::vec::IndexVec<rustc_target::abi::VariantIdx, rustc_middle::ty::VariantDef>::iter_enumerated::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:13668 ~ rustc_borrowck[bbba]::do_mir_borrowck), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [(rustc_middle::mir::Promoted, &rustc_middle::mir::Body), std::iter::Enumerate<std::slice::Iter<rustc_middle::mir::Body>>, [closure@rustc_index::vec::IndexVec<rustc_middle::mir::Promoted, rustc_middle::mir::Body>::iter_enumerated::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:7570 ~ rustc_typeck[4ccc]::coherence::inherent_impls_overlap::{impl#0}::impls_have_common_items), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [(&rustc_span::Symbol, &&rustc_middle::ty::AssocItem), std::slice::Iter<(rustc_span::Symbol, &rustc_middle::ty::AssocItem)>, [closure@rustc_data_structures::sorted_map::SortedIndexMultiMap<u32, rustc_span::Symbol, &rustc_middle::ty::AssocItem>::iter::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:7573 ~ rustc_typeck[4ccc]::coherence::inherent_impls_overlap::{impl#0}::check_for_common_items_in_impls), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [(&rustc_span::Symbol, &&rustc_middle::ty::AssocItem), std::slice::Iter<(rustc_span::Symbol, &rustc_middle::ty::AssocItem)>, [closure@rustc_data_structures::sorted_map::SortedIndexMultiMap<u32, rustc_span::Symbol, &rustc_middle::ty::AssocItem>::iter::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:3085 ~ rustc_borrowck[bbba]::nll::populate_polonius_move_facts), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:6994 ~ core[ef75]::iter::adapters::map::{impl#2}::next), const_param_did: None }), substs: [(rustc_middle::mir::Local, &rustc_mir_dataflow::move_paths::MovePathIndex), std::iter::Enumerate<std::slice::Iter<rustc_mir_dataflow::move_paths::MovePathIndex>>, [closure@rustc_index::vec::IndexVec<rustc_middle::mir::Local, rustc_mir_dataflow::move_paths::MovePathIndex>::iter_enumerated::{closure#0}]] }
MirSource { instance: Item(WithOptConstParam { did: DefId(0:398 ~ tracing_core[5d90]::field::{impl#35}::field::{closure#0}), const_param_did: None }), promoted: None } Instance { def: Item(WithOptConstParam { did: DefId(2:3041 ~ core[ef75]::cmp::impls::{impl#9}::eq), const_param_did: None }), substs: [ReErased, ReErased, str, str] }

[tmiasko]

Thanks!

r=me with pull request description updated.

[cjgillot]

@bors r=tmiasko

[bors]

📌 Commit 86645c9 has been approved by tmiasko

It is now in the queue for this repository.