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

@rustbot

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.

@oli-obk

cc @oli-obk

@rustbot

Can I give this a go?

@rustbot claim

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

// 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.

@bjorn3

@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.

@bjorn3

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.

// 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;
    }
[...]

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.

I forgot that this problem was only with simd_shuffle.

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

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.

Rollup merge of rust-lang#67598 - jumbatm:issue67557_simd_shuffle, r=…

Auto merge of #67598 - jumbatm:issue67557_simd_shuffle, r=oli-obk

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

Centril commented on Dec 23, 2019

jumbatm commented on Dec 23, 2019

jumbatm commented on Dec 23, 2019

bjorn3 commented on Dec 23, 2019

wesleywiser commented on Dec 24, 2019

jumbatm commented on Dec 24, 2019

bjorn3 commented on Dec 24, 2019

jumbatm commented on Dec 24, 2019

bjorn3 commented on Dec 24, 2019

bjorn3 commented on Dec 24, 2019

jumbatm commented on Dec 25, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions