Ban ArrayToPointer and MutToConstPointer from runtime MIR

[scottmcm]

Zulip conversation: https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/CastKind.3A.3APointerCoercion.20in.20Runtime.20MIR/near/443955195

Apparently MIR borrowck cares about at least one of these for checking variance.

In runtime MIR, though, there's no need for them as PtrToPtr does the same thing.

(Banning them simplifies passes like GVN that no longer need to handle multiple cast possibilities.)

r? mir

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

[RalfJung]

(Banning them simplifies passes like GVN that no longer need to handle multiple cast possibilities.)

How that? Can't GVN use an "or" pattern to cover all these cases together?

To me, banning them seems more complicated than simply allowing them.

[cjgillot]

Why not inside 'cleanup_post_borrowck' ?

[scottmcm]

Because I was thinking of it as part of Analysis->Runtime, and that pass is inside Analysis Initial->PostCleanup.

I could move it there and make it part of the https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/syntax/enum.AnalysisPhase.html#variant.PostCleanup disallow list if you'd prefer? (I don't have particularly strong opinions about exactly when it should happen, so I'll take your advice here.)

[scottmcm]

I was thinking about this more, and decided that it's easier to write the why comment when it's in cleanup_post_borrowck, so I moved it there, and adjusted them to be banned starting in Analysis(PostCleanup) rather than just in Runtime(..).

[scottmcm]

How that? Can't GVN use an "or" pattern to cover all these cases together?

I see it less as being about the code that's typed are more about "is it clear how you should think about things".

For example, today GVN groups these three here

rust/compiler/rustc_mir_transform/src/gvn.rs

Lines 575 to 579 in f158600

	CastKind::PointerCoercion(
	ty::adjustment::PointerCoercion::MutToConstPointer
	| ty::adjustment::PointerCoercion::ArrayToPointer
	| ty::adjustment::PointerCoercion::UnsafeFnPointer,
	) => {

but then only has PtrToPtr and MutToConstPointer in

rust/compiler/rustc_mir_transform/src/gvn.rs

Lines 1168 to 1171 in f158600

	if let PtrToPtr | PointerCoercion(MutToConstPointer) = kind
	&& let Value::Cast { kind: inner_kind, value: inner_value, from: inner_from, to: _ } =
	*self.get(value)
	&& let PtrToPtr | PointerCoercion(MutToConstPointer) = inner_kind

Is that right? Should the former have MutToConstPointer in the same arm as PtrToPtr? Should the latter have ArrayToPointer in it too? I don't know.

I think it's much simpler when the answer to those things is "as it says on CastKind and MirPhase::Runtime, you only need PtrToPtr here; ignore the rest".

[RalfJung]

I think it's much simpler when the answer to those things is "as it says on CastKind and MirPhase::Runtime, you only need PtrToPtr here; ignore the rest".

What about grouping all of these together in a single CastKind variant PtrToPtr(PtrCastKind), and declaring that operationally, the PtrCastKind makes no difference, it just exists for borrowck? Then it's easy to consistently match on it and ignore the irrelevant details in mir-opts.

This is not a strong opinion though, I just feel like this PR is using the MIR validation to work around CastKind just not being of the right shape for some of its consumers.

[RalfJung]

compiler/rustc_const_eval/src/interpret/cast.rs should then probably also explicitly panic on these casts.

[RalfJung]

Ah, we have have this inner enum, PointerCoercion... but that enum is a pretty poor fit for runtime MIR in general, it is designed for coercion generation way earlier in the compiler. I wonder if we'd be better off just not using it in MIR... but I have no idea how this is relevant for borrowck.

[scottmcm]

but I have no idea how this is relevant for borrowck.

I first tried just banning these from MIR altogether, but doing that meant there were some missing errors in one of the borrowck tests. I don't have the failure handy, but I think it was this one:

rust/tests/ui/nll/type-check-pointer-coercions.rs

Lines 15 to 17 in f158600

	fn mut_to_const<'a, 'b>(x: *mut &'a i32) -> *const &'b i32 {
	x //~ ERROR
	}

So it's checks about lifetimes inside the pointee, I guess?

[RalfJung]

I would expect the lifetimes in that test to be a red herring -- mut_to_const is the key. Someone has to reject that as a coercion (but allow it as a cast).

[RalfJung]

Ah no never mind, I shouldn't write comments when I am too tired...

on a mut-to-const implicit coercion, the type needs to stay the same, and the lifetime equality part of "the same" is checked by borrowck.

[scottmcm]

but that enum is a pretty poor fit for runtime MIR in general, it is designed for coercion generation way earlier in the compiler. I wonder if we'd be better off just not using it in MIR...

So I agree with this, but I'm not sure how to action on it. I don't really want to change https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/thir/enum.ExprKind.html#variant.PointerCoercion, and if we're stuck representing the distinction in MIR anyway, I don't know how helpful restructuring it would be.

What I've grokked here is that early MIR does need this distinction for borrowchecking coercions, but that coercion vs cast distinction is irrelevant in runtime MIR. So it doesn't sound that bad to me to just ban the borrowck-parts in later MIR phases, as I'm doing here? Even if we rearranged these cases to different CastKinds, I think I'd still rather just normalize things so there's fewer things MIR consumers have to deal with.

[RalfJung]

I don't have a concrete proposal either, and it seems you considered the alternatives, so my concern isn't blocking. I feel like there's got to be a better way but don't feel strongly enough about it to try and find it. ;)

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[saethlin]

I'm okay with this change because it makes the runtime MIR dialect more sensible. I don't know how much this actually matters in practice, clearly it lets you clean up MIR transforms if you know what you're doing. But the fact that the enum variants still exist because all MIR dialects are the same type still makes implementing transforms confusing.

@bors r+

[bors]

📌 Commit 64bba35 has been approved by saethlin

It is now in the queue for this repository.

[fmease]

#126669 (comment)
@bors r-

[scottmcm]

Rebased and re-blessed.

@bors r=saethlin rollup=iffy (already failed in a rollup once)

[bors]

📌 Commit e04e351 has been approved by saethlin

It is now in the queue for this repository.

[bors]

⌛ Testing commit e04e351 with merge 3d5d7a2...

[bors]

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 3d5d7a2 to master...

[rust-timer]

Finished benchmarking commit (3d5d7a2): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.4%, secondary 3.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.9%	[3.1%, 5.4%]	3
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 692.485s -> 691.149s (-0.19%)
Artifact size: 323.83 MiB -> 323.84 MiB (0.01%)