Index argument for simd_insert no longer a const

[bjorn3]

I tried this code:

let a = _mm_setr_epi16(0, 1, 2, 3, 4, 5, 6, 7);                                                 
let r = _mm_insert_epi16(a, 9, 0);                                                              
let e = _mm_setr_epi16(9, 1, 2, 3, 4, 5, 6, 7);                                                 
assert_eq_m128i(r, e);

I expected to see this happen: The index argument 9 is given as direct constant to the simd_insert intrinsic.

Instead, this happened: The cast of the index argument to u32 is stored in a local and then this local is passed to simd_insert. This causes cg_clif to error out as it requires a constant.

#[rustc_legacy_const_generics(2)]
pub unsafe fn _mm_insert_epi16<const IMM8: i32>(a: __m128i, i: i32) -> __m128i {                    
    static_assert_imm3!(IMM8);                                                                      
    transmute(simd_insert(a.as_i16x8(), IMM8 as u32, i as i16))                                     
}

[src/constant.rs:450] fx.mir = Body {
    basic_blocks: [
        BasicBlockData {
            statements: [
                StorageLive(_3),
                StorageLive(_4),
                StorageLive(_5),
                _5 = _1,
                StorageLive(_9),
                StorageLive(_10),
                _10 = move _5,
                _9 = _10,
                StorageDead(_10),
            ],
            terminator: Some(
                Terminator {
                    source_info: // ...
                    kind: _4 = std::intrinsics::transmute::<std::arch::x86_64::__m128i, core::core_arch::simd::i16x8>(move _9) -> bb3,
                },
            ),
            is_cleanup: false,
        },
        BasicBlockData {
            statements: [
                StorageDead(_7),
                StorageDead(_6),
                StorageDead(_4),
            ],
            terminator: Some(
                Terminator {
                    source_info: // ...
                    kind: _0 = std::intrinsics::transmute::<core::core_arch::simd::i16x8, std::arch::x86_64::__m128i>(move _3) -> bb2,
                },
            ),
            is_cleanup: false,
        },
        BasicBlockData {
            statements: [
                StorageDead(_3),
            ],
            terminator: Some(
                Terminator {
                    source_info: // ...
                    kind: return,
                },
            ),
            is_cleanup: false,
        },
        BasicBlockData {
            statements: [
                StorageDead(_9),
                StorageDead(_5),
                StorageLive(_6),
                _6 = const IMM8 as u32 (Misc),
                StorageLive(_7),
                StorageLive(_8),
                _8 = _2,
                _7 = move _8 as i16 (Misc),
                StorageDead(_8),
            ],
            terminator: Some(
                Terminator {
                    source_info: // ...
                    kind: _3 = core::core_arch::simd_llvm::simd_insert::<core::core_arch::simd::i16x8, i16>(move _4, move _6, move _7) -> bb1,
                },
            ),
            is_cleanup: false,
        },
    ],
    phase: Optimization,
    source: MirSource {
        instance: Item(
            WithOptConstParam {
                did: DefId(2:13246 ~ core[8231]::core_arch::x86::sse2::_mm_insert_epi16),
                const_param_did: None,
            },
        ),
        promoted: None,
    },
    local_decls: [
        LocalDecl {
            ty: std::arch::x86_64::__m128i,
            // ...
        },
        LocalDecl {
            ty: std::arch::x86_64::__m128i,
            // ...
        },
        LocalDecl {
            ty: i32,
            // ...
        },
        LocalDecl {
            ty: core::core_arch::simd::i16x8,
            // ...
        },
        LocalDecl {
            ty: core::core_arch::simd::i16x8,
            // ...
        },
        LocalDecl {
            ty: std::arch::x86_64::__m128i,
            // ...
        },
        LocalDecl {
            ty: u32,
            // ...
        },
        LocalDecl {
            ty: i16,
            // ...
        },
        LocalDecl {
            ty: i32,
            // ...
        },
        LocalDecl {
            ty: std::arch::x86_64::__m128i,
            // ...
        },
        LocalDecl {
            ty: std::arch::x86_64::__m128i,
            // ...
        },
    ],
    var_debug_info: [
        VarDebugInfo {
            name: "a",
            source_info: // ...
            value: _1,
        },
        VarDebugInfo {
            name: "i",
            source_info: // ...
            value: _2,
        },
        VarDebugInfo {
            name: "self",
            source_info: // ...
            value: _5,
        },
        VarDebugInfo {
            name: "self",
            source_info: // ...
            value: _10,
        },
    ],
    required_consts: [
        const core::core_arch::macros::ValidateConstImm::<IMM8, 0_i32, 7_i32>::VALID,
    ],
    // ...
}
[src/constant.rs:451] operand = move _6

Meta

rustc --version --verbose:

rustc 1.54.0-nightly (881c1ac40 2021-05-08)
binary: rustc
commit-hash: 881c1ac408d93bb7adaa3a51dabab9266e82eee8
commit-date: 2021-05-08
host: x86_64-unknown-linux-gnu
release: 1.54.0-nightly
LLVM version: 12.0.0

[bjorn3]

I worked around this in bjorn3/rustc_codegen_cranelift@bcc68c2 by walking the MIR to find a single defining use of the local that should contain the constant argument.

[oli-obk]

I guess we should give simd_insert some generic const arguments? So instead of

fn simd_insert<T, E>(x: T, idx: u32, y: E) -> T;

we would have

fn simd_insert<T, E, const IDX: u32>(x: T, y: E) -> T;

does that fit how that index works? cc @lqd does SIMD have a team to ping?

[lqd]

There is a zulip stream for stdarch but it's pretty much inactive (opening issues in https://github.com/rust-lang/stdarch does work).

For pings, I believe @Amanieu would be the main person to contact for stdarch.

For this issue in particular, I do wonder if simd_insert may need a similar treatment to what Ralf did with simd_shuffle recently (but simpler; and for different reasons obviously) if this needs to be fixed ?

[Nugine]

Using inline_const may fix this issue.

transmute(simd_insert(a.as_i16x8(), const { IMM8 as u32 }, i as i16))

[oli-obk]

I think we should still enforce this at the signature level by making it a const generic parameter. If anyone wants to be mentored on this, I'd be happy to do that.

We'd roll it out with minimum annoyance by creating a

extern "rust-intrinsic" {
    fn simd_insert_new<T, U, const N: u32>(t: T, u: U) -> T
}

that we can then add incrementally to all use sites (mostly the stdsimd submodule). Updating the submodule at the same time as the intrinsic would be super annoying to do, so let's not ^^

[lqd]

@oli-obk will it be possible to cast the i32 const generic parameter that the intrinsics use, to this simd_insert_new's u32 const generic parameter without locals ? (would it require adding post-monomorphized conversion at each simd_insert call site, or generic_const_exprs ?)

[oli-obk]

we could just make the const N: u32 be i32 instead, would that work? or are both signed and unsigned versions needed?

[lqd]

I'm not sure, I thought simd_insert was one of the LLVM extern C intrinsics that stdarch calls directly, but it's a "platform-intrinsic" so we may have more leeway about its signature.

[oli-obk]

I'm not sure, I thought simd_insert was one of the LLVM extern C intrinsics that stdarch calls directly, but it's a "platform-intrinsic" so we may have more leeway about its signature.

there is no direct calls anywhere. We are generating the llvm intrinsic call during codegen. We can pick whatever signature we need from the Rust side and translate during LLVM codegen.

[bjorn3]

For all simd_* intrinsics this has been fixed now, though it is not enforced in any backend other than cg_clif. For LLVM intrinsics there are still some cases remaining however. (search for mir_operand_get_const_val in cg_clif's source)