[MIR] simd_shuffle codegen uses self.instance as const_eval_promoted source

[bjorn3]

rust/src/librustc_codegen_ssa/mir/block.rs

Line 628 in a916ac2

let c = bx.tcx().const_eval_promoted(self.instance, promoted);

It should use Instance::new(def_is, self.monomorphize(substs). (substs is the second field of StaticKind::Promoted.) While both instances are normally equal, they can differ after running the mir inliner. In that case the promoted will refer to the inlined function.

@rustbot modify labels: +A-mir +C-bug +requires-nightly

This issue has been assigned to @jumbatm via this comment.

[Centril]

cc @oli-obk

[jumbatm]

Can I give this a go?

@rustbot claim

[jumbatm]

Do you have any specific tests in mind I should add to check for the correct behaviour?

[bjorn3]

// compile-flags:-Zmir-opt-level=3

#[inline(never)]
fn assert_1234(x: &[u8]) {
    assert_eq!(x, &[1, 2, 3, 4]);
}

#[inline(always)]
fn ret_slice() -> &'static [u8] {
    &[1, 2, 3, 4]
}

fn main() {
    let promoted: &'static [u8] = &[0, 0, 0, 0];
    let slice: &'static [u8] = ret_slice();
    assert_1234(slice);
}

should be a test if the mir inliner would to inline ret_slice. Somehow it doesn't though.

[wesleywiser]

@bjorn3 You might be running into this unfortunate check

rust/src/librustc_mir/transform/inline.rs

Line 106 in 6253754

&& self_node_id.as_u32() < callee_node_id.as_u32()

Try moving ret_slice below main.

[jumbatm]

Wow, that's hilarious. With just the changes @bjorn3 suggested, the inlining still doesn't work properly, but doesn't seem to break any existing stuff. But I've just tried this and yeah, putting ret_slice below main has made the MIR inliner work.

[bjorn3]

// compile-flags:-Zmir-opt-level=3

fn main() {
    let promoted: &'static [u8] = &[0, 0, 0, 0];
    let slice: &'static [u8] = ret_slice();
    assert_1234(slice); assert_0000(promoted);
}

#[inline(never)]
fn assert_1234(x: &[u8]) {
    assert_eq!(x, &[1, 2, 3, 4]);
}

#[inline(never)]
fn assert_0000(x: &[u8]) {
    assert_eq!(x, &[0, 0, 0, 0]);
}

#[inline(always)]
fn ret_slice() -> &'static [u8] {
    &[1, 2, 3, 4]
}

This does inline, but somehow no assertion fires with current rustc. The printed MIR doesn't seem to show the the def_id from which the promoted originates, so I don't know what the problem is.

[...]
    bb0: {
        _2 = &(promoted[0]: [u8; 4]);
        _1 = move _2 as &[u8] (Pointer(Unsize));
        _6 = &(promoted[0]: [u8; 4]);
        _3 = move _6 as &[u8] (Pointer(Unsize));
        StorageLive(_4);
        _4 = const assert_1234(move _3) -> bb1;
    }

    bb1: {
        StorageDead(_4);
        StorageLive(_5);
        _5 = const assert_0000(move _1) -> bb2;
    }
[...]

[jumbatm]

Unless I'm misunderstanding something, isn't this the correct behaviour? Both of those assertions are true, and swapping slice and promoted around in the assertions correctly triggers an assertion failure.

[bjorn3]

I forgot that this problem was only with simd_shuffle.

[bjorn3]

I got a working test:

#![feature(platform_intrinsics, repr_simd)]

extern "platform-intrinsic" {
    fn simd_shuffle2<T, U>(x: T, y: T, idx: [u32; 2]) -> U;
}

#[repr(simd)]
#[derive(Debug, PartialEq)]
struct Simd2(u8, u8);

fn main() {
    unsafe {
        let p_res: Simd2 = simd_shuffle2(Simd2(10, 11), Simd2(12, 13), [0, 1]);
        let a_res: Simd2 = inline_me();

        assert_10_11(p_res);
        assert_10_13(a_res);
    }
}

#[inline(never)]
fn assert_10_11(x: Simd2) {
    assert_eq!(x, Simd2(10, 11));
}

#[inline(never)]
fn assert_10_13(x: Simd2) {
    assert_eq!(x, Simd2(10, 13));
}


#[inline(always)]
unsafe fn inline_me() -> Simd2 {
    simd_shuffle2(Simd2(10, 11), Simd2(12, 13), [0, 3])
}

$ rustc ./simd_shuffle_promotion_problem.rs && ./simd_shuffle_promotion_problem
$ rustc -Zmir-opt-level=2 ./simd_shuffle_promotion_problem.rs && ./simd_shuffle_promotion_problem
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Simd2(10, 11)`,
 right: `Simd2(10, 13)`', ./simd_shuffle_promotion_problem.rs:28:5

[jumbatm]

Ah, that's a good test case. Yeah, that's definitely not the expected behaviour.

While messing about with your example, I also managed to come across an ICE with this slightly shorter version:

#![feature(platform_intrinsics, repr_simd)]

extern "platform-intrinsic" {
    fn simd_shuffle2<T, U>(x: T, y: T, idx: [u32; 2]) -> U;
}

#[repr(simd)]
#[derive(Debug, PartialEq)]
struct Simd2(u8, u8);

fn main() {
    unsafe {
        let _: Simd2 = inline_me(); 
    }
}

#[inline(always)]
unsafe fn inline_me() -> Simd2 {
    simd_shuffle2(Simd2(10, 11), Simd2(12, 13), [0, 1])
}

causes an ICE currently due to an internal index out of range.

The good news is that the changes you suggested fixes both of these test cases. Good spot :) I'm putting up the PR now.