ICE with mir-opt-level=2 in program with scalar pair value

[osa1]

fn test(this: ((u8, u8),)) {
    assert!((this.0).1 == 0);
}

fn main() {
    test(((1, 2),));
}

Build:

$ /home/omer/rust/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc --edition 2018 test2.rs -Zmir-opt-level=2

ICE:

thread 'rustc' panicked at 'index out of bounds: the len is 1 but the index is 1', src/librustc_mir/transform/const_prop.rs:648:42
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.41.0-dev running on x86_64-unknown-linux-gnu

note: compiler flags: -Z mir-opt-level=2

[osa1]

#67015 improves the situation a little bit but it's still not enough. The problem is (as accurately identified by @tmiasko in #67015) when we have something like ((1u8, 2u8),) the layout of it says this has one field, but mir value of evaluating this is a ScalarPair, so we can't propagate value of this expression correctly using the type because the type says this has one field but evaluation of it is two values.

Two options:

• Ignore types entirely, if something evaluates to a pair then represent it as a pair regardless of the type.
• Use ABI of the layout

Either way, this will effect resolution of #66971 too. @oli-obk says in the issue:

If there are three fields, we need three entries even if one of them is zst

I don't know how to fix this while also doing what @oli-obk says above. Currently I use types so in PR #67015 the MIR we generate for ((), u8, u8) after const-prop is (const Scalar(<ZST>) : (), const 0u8, const 0u8), but if we do any of the options above we'll get (const Scalar(<ZST>) : const 0u8, const 0u8) (note that the type changed, we no longer have the zst ()).

@oli-obk @wesleywiser does this make sense? How should I proceed here?

I think first option would allow more constant propagation. Both options would change logical type of values in some cases.

[oli-obk]

Ok, so there are multiple things at play here. We have a total of 3 representations that ((1, 2),) can be represented as:

1. A ty::Const via ConstValue::ByRef, by creating an Allocation (virtual const eval memory) that contains the bits of ((1, 2),) as they would show up on the target at runtime
2. An Rvalue::Aggregate with a single element of either const (1, 2) or of an Operand::Move from a local that contains this value
3. An Immediate::ScalarPair. This can only happing during const evaluation and will never show up in MIR or constants. It is represented as a ScalarPair, because that's the layout of it.

Our current situation is that we convert scalarpairs directly to two element aggregates. This is wrong though, because while the memory representation of a value may be a scalar pair, the MIR needs to build all levels of a value irrelevant of its memory representation. The MIR just looks at the fields that are there, not at the layout.

Side note: the correct way to get a field from an ImmTy is to turn it into an OpTy and call https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir/interpret/struct.InterpCx.html#method.operand_field . We could pull out the immediate specific logic (https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_mir/interpret/operand.rs.html#406) into a separate function if we end up needing to access fields of ImmTys. I don't believe we'll end up in a happy situation if we start doing this, because as shown in option 2. above, we'd need to generate more locals with the component values.

So... My suggestion is to instead always create an Rvalue::Use(Operand::Constant(...)). So we keep doing what we're doing for Scalars (create a ConstValue::Scalar), because that works for all types with Scalar layout. For other types there are a few things that can be done:

• &str and &[u8] become ConstValue::Slice
• everything else becomes ConstValue::ByRef by moving their content into an Allocation.

And the fun thing about it, you don't have to write any code. You can just make

rust/src/librustc_mir/const_eval.rs

Line 54 in 7b482cd

fn op_to_const<'tcx>(

public and call it. This means we stop calling read_immediate in

rust/src/librustc_mir/transform/const_prop.rs

Lines 624 to 627 in 7b482cd

	// FIXME> figure out what tho do when try_read_immediate fails
	let imm = self.use_ecx(source_info, |this| {
	this.ecx.try_read_immediate(value)
	});

and instead call op_to_const there.

Now this may cause two other problems:

1. We may regress our peak memory usage, because we may suddenly create (and intern) a lot of Allocations that are quickly optimized out. I don't know yet how to address this. I'm not sure if this can actually happen, but I presume that [0; 999999999] in the MIR as Rvalue::Repeat will end up as a constant with its value. Or constructing a super large union with a small variant will allocate the entire union's memory. I think these cause a lot of memory usage in llvm anyway, and there are ways we can resolve some of these situations with more fancy const eval Allocation tricks (we already have a trick for all bits being Undef)
2. op_to_const panics if the allocation is not already interned. We don't want to eagerly intern all allocations in an OpTy though. I think we can avoid overeager interning by calling read_immediate and if we got an ImmTy, make the OpTy from that ImmTy and use the original OpTy otherwise. This makes sure we "normalize" the OpTy to its ImmTy variant, thus eliminating all MPlaceTy that are not strictly necessary for representation. After this normalization we can eagerly intern all AllocIds found in the OpTy and then call op_to_const