Miri/CTFE function call checks should only look at layout, not the type

[RalfJung]

Since #61814 we have a strange condition in the Miri/interpreter code that checks if a function got called with appropriate arguments: !callee_layout.abi.is_uninhabited() got replaced by !self.tcx.is_ty_uninhabited_from_any_module(ty). That's really strange, the layout should contain all the information we need for execution, and module boundaries certainly shouldn't matter for the call ABI. So I don't think this change is right, at least not for Miri. Unfortunately, there is 0 explanation of the "why" in that PR and in the code. It also does fix an ICE.

Is this a case of running the interpreter on untyped code? Or why does looking at the layout not work?

Cc @oli-obk @eddyb @varkor

[oli-obk]

This is entirely my fault and also entirely unnecessary for fixing the ICE. I just believed this to be a useful cleanup: #61814 (comment)

The reason is that computing the layout in a cached way (via layout_of_local) requires that we actually have an IndexVec of locals, which we don't have for const prop. The previous code was 22fc7db so if you prefer that, I'm fine with moving there, I just thought it to be better to fix the problem at the layout checking place instead of at the layout computation place.

[RalfJung]

I'm afraid I disagree with that being a cleanup -- AFAIK the type-based tests exist for surface language stuff, like exhaustiveness check and whatnot, also taking into account semver considerations. But for the interpreter I'd like to be sure that we use the layout information that will also be used by codegen.

The reason is that computing the layout in a cached way (via layout_of_local) requires that we actually have an IndexVec of locals, which we don't have for const prop.

const_prop making a mess for everyone else, again. :(

That other code seems okay to me, the comment is confusing though. What does that have to do with "constants being trivial"? And "don't allocate any locals" sounds like we wouldn't make them ByRef, which is not at all what this is about. But if that explains that frame.locals might be empty when we are in const_prop, I'd r+ that.

[oli-obk]

const_prop making a mess for everyone else, again. :(

actually, @wesleywiser has just made some changes there or is in the process of further changes. We may be able to revert #61814 after those changes are through because we'll have an actual stack frame available

[RalfJung]

That would be great! I thought there were some performance reasons for this.

[oli-obk]

There were strong performance reasons in the past. We alleviated most of them, but there are some left: https://perf.rust-lang.org/compare.html?start=d4d5d67c1c20b9599c812ab4d926ab4fa9fe6935&end=b9376e62aea7c07ac50bf6929abf4931611b10a9

not even 2% regression in artificial const eval benchmarks are fine with me for the enormous reduction in code complexity and duplication. We may be able to find further improvements that help const prop and const eval now that they have started sharing more code.

[RalfJung]

Besides this issue here, do we carry existing hacks that we can remove with the more well-behaved const_prop?

[oli-obk]

I think there are some hacks about fallback behaviour on empty stacks