MIR: use HirId instead of NodeId to avoid cycles while inlining

[ljedrz]

I wanted to see if I could limit the number of uses of NodeId when HirId is available and I saw that some of the MIR Inliner code could use Span instead of NodeId, not unlike in #71197.

If I'm understanding the reason for not calling optimized_mir in incremental builds here correctly, this change could also allow us to do so.

This change could affect performance, so if this approach makes sense, a perf run is probably a good idea.

[rust-highfive]

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

[varkor]

This seems reasonable, but I'm not familiar enough with the subtleties to be sure, so reassigning to the reviewer of the linked PR.

r? @ecstatic-morse

[ecstatic-morse]

#71197 only needed to sort things so that error messages would be produced in source order. In this case, we need to define an arbitrary order for all callable functions to avoid inlining cycles. Why not use the HirId itself for this?

Also, I don't believe perf runs have MIR inlining enabled. If that's true, any slowdown caused by looking up all these spans wouldn't have been visible.

[ljedrz]

If the order can be arbitrary then it makes sense to just use HirIds - I'll run the tests. Wouldn't this mean it can work in incremental builds too? HirIds are stable after all.

[ecstatic-morse]

I don't know. The goal is to have the exact same optimizations applied across incremental runs. If you can prove that is indeed what happens, then have at it. However, I suspect that if it were that simple, HirId would have been used in the first place.

[bjorn3]

HirIds are stable after all.

A part of a HirId is a DefId, which is basically an interned DefPath. While a DefPath is guaranteed to be stable, DefIds are just numbered sequentially starting from 0 in order of lowering items to hir:

rust/src/librustc_hir/definitions.rs

Lines 35 to 45 in df768c5

	fn allocate(&mut self, key: DefKey, def_path_hash: DefPathHash) -> DefIndex {
	let index = {
	let index = DefIndex::from(self.index_to_key.len());
	debug!("DefPathTable::insert() - {:?} <-> {:?}", key, index);
	self.index_to_key.push(key);
	index
	};
	self.def_path_hashes.push(def_path_hash);
	debug_assert!(self.def_path_hashes.len() == self.index_to_key.len());
	index
	}

This means that if functions are reordered, comparing the order of the HirId will give a different result without the incremental cache system knowing it. This will likely not be a problem at the moment though, as I think the changing span would be enough to cause a recompilation. However if both functions are given DUMMY_SPAN by a proc macro this invalidation won't happen anymore.

[ecstatic-morse]

@bors r+

@ljedrz Feel free to explore the interaction between incremental and the current approach to avoiding inlining cycles. Note that more robust approach to MIR inlining in cyclic call graphs would eliminate that issue, along with several others. This is not a simple problem, however. See #43542.

[bors]

📌 Commit 3c455fe has been approved by ecstatic-morse