Miscompilation of tuple arguments with mir-opt-level=2

[bytwise]

This reduced testcase is getting miscompiled with mir-opt-level=2. I bisected it to #64890.

fn encode(this: ((), u8, u8)) {
    // this assert panics although this.2 is 0
    assert!(this.2 == 0);
}

fn emit() {
    encode(((), 0, 0));
}

fn emit_inc() -> Box<()> {
    emit();
    Box::new(())
}

fn main() {
    emit_inc();
}

Rust version:

rustc 1.41.0-nightly (4007d4ef2 2019-12-01)
binary: rustc
commit-hash: 4007d4ef26eab44bdabc2b7574d032152264d3ad
commit-date: 2019-12-01
host: x86_64-apple-darwin
release: 1.41.0-nightly
LLVM version: 9.0

[Centril]

cc @wesleywiser @oli-obk

[osa1]

Slightly simpler reproducer fails the same way:

fn encode(this: ((), u8, u8)) {
    // this assert panics although this.2 is 0
    assert!(this.2 == 0);
}

fn main() {
    encode(((), 0, 0));
}

[osa1]

I may be talking nonsense (first time looking at a mir dump), but I think in main:

fn  main() -> () {
    let mut _0: ();                      // return place in scope 0 at test.rs:6:11: 6:11
    let _1: ();                          // in scope 0 at test.rs:7:5: 7:23
    let mut _2: ((), u8, u8);            // in scope 0 at test.rs:7:12: 7:22

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 0 at test.rs:7:5: 7:23
        StorageLive(_2);                 // bb0[1]: scope 0 at test.rs:7:12: 7:22
        (_2.0: ()) = const Scalar(0x00) : (); // bb0[2]: scope 0 at test.rs:7:12: 7:22
                                         // ty::Const
                                         // + ty: ()
                                         // + val: Value(Scalar(0x00))
                                         // mir::Constant
                                         // + span: test.rs:7:12: 7:22
                                         // + literal: Const { ty: (), val: Value(Scalar(0x00)) }
        (_2.1: u8) = const 0u8;          // bb0[3]: scope 0 at test.rs:7:12: 7:22
                                         // ty::Const
                                         // + ty: u8
                                         // + val: Value(Scalar(0x00))
                                         // mir::Constant
                                         // + span: test.rs:7:12: 7:22
                                         // + literal: Const { ty: u8, val: Value(Scalar(0x00)) }
        _1 = const encode(move _2) -> bb1; // bb0[4]: scope 0 at test.rs:7:5: 7:23
                                         // ty::Const
                                         // + ty: fn(((), u8, u8)) {encode}
                                         // + val: Value(Scalar(<ZST>))
                                         // mir::Constant
                                         // + span: test.rs:7:5: 7:11
                                         // + literal: Const { ty: fn(((), u8, u8)) {encode}, val: Value(Scalar(<ZST>)) }
    }

    bb1: {
        StorageDead(_2);                 // bb1[0]: scope 0 at test.rs:7:22: 7:23
        StorageDead(_1);                 // bb1[1]: scope 0 at test.rs:7:23: 7:24
        return;                          // bb1[2]: scope 0 at test.rs:8:2: 8:2
    }
}

_2.2 should've been initialized similar to _2.1 (something like (_2.2: u8) = const 0u8;), but it's not.

[osa1]

Looking at MIR dumps, before constant propagation this is how _2 is initialized:

        _2 = (move _3, const 0u8, const 0u8); // bb0[4]: scope 0 at test.rs:7:12: 7:22
                                         // ty::Const
                                         // + ty: u8
                                         // + val: Value(Scalar(0x00))
                                         // mir::Constant
                                         // + span: test.rs:7:17: 7:18
                                         // + literal: Const { ty: u8, val: Value(Scalar(0x00)) }
                                         // ty::Const
                                         // + ty: u8
                                         // + val: Value(Scalar(0x00))
                                         // mir::Constant
                                         // + span: test.rs:7:20: 7:21
                                         // + literal: Const { ty: u8, val: Value(Scalar(0x00)) }

After:

        _2 = (const Scalar(0x00) : (), const 0u8); // bb0[4]: scope 0 at test.rs:7:12: 7:22
                                         // ty::Const
                                         // + ty: ()
                                         // + val: Value(Scalar(0x00))
                                         // mir::Constant
                                         // + span: test.rs:7:12: 7:22
                                         // + literal: Const { ty: (), val: Value(Scalar(0x00)) }
                                         // ty::Const
                                         // + ty: u8
                                         // + val: Value(Scalar(0x00))
                                         // mir::Constant
                                         // + span: test.rs:7:12: 7:22
                                         // + literal: Const { ty: u8, val: Value(Scalar(0x00)) }

So the last field disappears.

[osa1]

I think the bug is in these lines:

    fn replace_with_const(
        &mut self,
        rval: &mut Rvalue<'tcx>,
        value: Const<'tcx>,
        source_info: SourceInfo,
    ) {
        ...
                Immediate::ScalarPair(
                    ScalarMaybeUndef::Scalar(one),
                    ScalarMaybeUndef::Scalar(two)
                ) => {
                    let ty = &value.layout.ty.kind;
                    if let ty::Tuple(substs) = ty {
                        debug!("substs: {:#?}", substs);
                        *rval = Rvalue::Aggregate(
                            Box::new(AggregateKind::Tuple),
                            vec![
                                self.operand_from_scalar(
                                    one, substs[0].expect_ty(), source_info.span
                                ),
                                self.operand_from_scalar(
                                    two, substs[1].expect_ty(), source_info.span
                                ),
                            ],
                        );
                    }
        ...

In this program when visiting main substs is [ (), u8, u8 ] so the rvalue needs to be [substs[1], substs[2]] not [substs[0], substs[1]].

[osa1]

Confirmed that commenting out this block of code fixes the issue.

Not sure how to proceed. If we want to do this optimization for scalar pairs
then I think we'll have to filter out zero-sized types the value's type in
subst. Would that make sense?

[oli-obk]

Since we are making the decision on something that came from a layout, but then use the type to get at said field, we should probably figure out a layout based scheme for this. You can ask the layout for a field count, then iterate and use the field method to get a layout for each field. If it's zst, create a value from nothing, if not, fetch the next value from the scalar pair. If there are three fields, we need three entries even if one of them is zst

[osa1]

Thanks @oli-obk. I have a fix, PR coming.