Remove rustc_args_required_const attribute

[RalfJung]

Now that stdarch no longer needs it (thanks @Amanieu!), we can kill the rustc_args_required_const attribute. This means that lifetime extension of references to temporaries is the only remaining job that promotion is performing. :-)

r? @oli-obk
Fixes #69493

[rust-highfive]

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

[oli-obk]

Looks like we need to do the same transformation that was done to stdarch for SIMD shuffle?

[RalfJung]

I am confused... so aho-corasick is somehow directly calling these intrinsics?

The attribute was removed from the shuffle intrinsics with rust-lang/stdarch#1113, but it looks like they were special-cased in the promotion code so they got the magic treatment even without the attribute.

[oli-obk]

codegen does have a special case for these, yea. So... we need a const item with the indices and use that as the argument? Then bump stdsimd again and then we can do this PR?

[RalfJung]

Not just codegen, but promotion has a special case where it treats them as rustc_args_required_const even when that attribute is not present.

[RalfJung]

But yeah, I think the shuffle functions in stdarch need to become rustc_legacy_const_generics as well.

[RalfJung]

FWIW the error points here:

https://github.com/rust-lang/stdarch/blob/c2f21d24b938483d74e277d17fed81a6e7296a97/crates/core_arch/src/x86/avx2.rs#L298

[RalfJung]

Hm okay, this is a bit more tricky than I thought. The shuffle function cannot literally use the "legacy" attribute:

error: #[rustc_legacy_const_generics] functions must only have const generics

And moreover most (all?) calls are inside the same crate so the attribute does not even help...

The alternative is to fix all call sites of simd_shuffle to pass actual constants. I guess ideally the type checker would enforce this as part of a special check for these intrinsics?

[RalfJung]

I started seeing what it takes to fix this, and... it's a lot.^^ Here's my WIP patch. I don't like using inline_const (it is marked as incomplete_feature, we should not use those in the std library IMO); I feel the easiest way forward here might be a macro that generates code like

            const IDX: [u32; 32] = $idx;
            simd_shuffle32($x, $y, IDX)

The shuffle functions are AFAIK internal to stdarch, so we can pretty much do whatever we want here.

[RalfJung]

Yeah the macro approach looks pretty good, I think.

However, we'll need to figure out what to do with code like this that computes the mask based on other const-generic arguments...

[oli-obk]

However, we'll need to figure out what to do with code like this that computes the mask based on other const-generic arguments...

Oof. That is horrible. And the function is stable, too. The only way forward that we currently have is const_evaluatable_checked and that is nowhere near ready.

My proposal would be to compute all four constants as separate constants and do a runtime match on the generic arg and have four monomorphic calls this way.

You can keep the existing code by putting it in a const fn and calling that from each of the four constants

[RalfJung]

My proposal would be to compute all four constants as separate constants and do a runtime match on the generic arg and have four monomorphic calls this way.

Which "four constants"? There are 256 possible values for IMM8, aren't there?
I don't understand your proposal.^^

I think long-term the intrinsic itself should be changed to use const generics; this also ensures that we don't get these ICEs. Short-term we can model this with const generic wrapper like

pub fn simd_shuffle32<T, U, IDX: const [u32; 32]>(x: T, y: T) -> U { 
  extern "platform-intrinsic" {
    fn simd_shuffle32<T, U>(x: T, y: T, idx: [u32; 32]) -> U;
  }
  simd_shuffle32(x, y, IDX)
}

However, this complains that [u32; 32] can't be used as const generic yet (and I assume we'd need another incomplete_feature to get around that).

What I don't know is how to write functions like this that call such a const-generic shuffle-wrapper.

[oli-obk]

Which "four constants"? There are 256 possible values for IMM8, aren't there?
I don't understand your proposal.^^

Yes, but it is bitmasked to 0b11

What I don't know is how to write functions like this that call such a const-generic shuffle-wrapper.

That's what I was describing in my previous post. If you want to skip the runtime part and do it at compile time, you need the const_evaluatable_checked feature.

[RalfJung]

That's what I was describing in my previous post.

Yeah and I did not (and do not) understand the description. ;)

But I think I found another way. :D This macro:

#[allow(unused_macros)]
macro_rules! simd_shuffle8_param {
    ($x:expr, $y:expr, <const $imm:ident : $ty:ty> $idx:expr $(,)?) => {{
        struct ConstParam<const $imm: $ty>;
        impl<const $imm: $ty> ConstParam<$imm> {
            const IDX: [u32; 8] = $idx;
        }

        simd_shuffle8($x, $y, ConstParam::<$imm>::IDX)
    }}
}

Lets me write callers like

pub unsafe fn _mm256_shuffle_epi32<const MASK: i32>(a: __m256i) -> __m256i {
    static_assert_imm8!(MASK);
    let r: i32x8 = simd_shuffle8_param!(
        a.as_i32x8(),
        a.as_i32x8(),
        <const MASK: i32> [
            MASK as u32 & 0b11,
            (MASK as u32 >> 2) & 0b11,
            (MASK as u32 >> 4) & 0b11,
            (MASK as u32 >> 6) & 0b11,
            (MASK as u32 & 0b11) + 4,
            ((MASK as u32 >> 2) & 0b11) + 4,
            ((MASK as u32 >> 4) & 0b11) + 4,
            ((MASK as u32 >> 6) & 0b11) + 4,
        ],
    );
    transmute(r)
}

[RalfJung]

With this patch to stdarch, the bootstrap build completes again (or at least, aho-corasick builds; the full build is still ongoing).

@oli-obk @Amanieu would that be an okay avenue to take here?

[RalfJung]

@oli-obk I'd also like to have a reasonable error instead of the ICE when a shuffle argument is not a constant.

Currently, such an error is emitted here:

rust/compiler/rustc_mir/src/transform/promote_consts.rs

Line 795 in 1b30245

let msg = format!("argument {} is required to be a constant", index + 1);

But with promotion not having anything to do here any more, that makes little sense. What would be the right place to do that check? I thought it might be

rust/compiler/rustc_typeck/src/check/intrinsic.rs

Line 99 in 003b8ea

pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) {

but that function only checks the signature used in the extern block; we need something that checks each use site.

[Amanieu]

The patch to stdarch LGTM, but it looks incomplete (missing the ARM/AArch64 intrinsics for example).

[RalfJung]

Yeah I just wanted to get rustc to build for now. Once I figured out how to best make rustc error when the argument is not a const, I will apply this throughout stdarch.

[oli-obk]

but that function only checks the signature used in the extern block; we need something that checks each use site.

mir validation perhaps? or just codegen, which is already checking that i thought

[RalfJung]

Checking during codegen means we only check this when stdarch is used; I'd rather get the build errors in stdarch itself than waiting until some crate runs into the ICE.^^

MIR validation runs a lot, doesn't it? Seems excessive.
I was thinking of putting it into the lower-intrinsics pass, since at least that is already intrinsics-related...^^

[oli-obk]

Checking during codegen means we only check this when stdarch is used; I'd rather get the build errors in stdarch itself than waiting until some crate runs into the ICE.^^

MIR validation runs a lot, doesn't it? Seems excessive.
I was thinking of putting it into the lower-intrinsics pass, since at least that is already intrinsics-related...^^

Yea, that opt is always run, even without optimizations, so 👍

[RalfJung]

Yea, that opt is always run, even without optimizations, so +1

All right, done.

The stdarch PR is on its way at rust-lang/stdarch#1160, but some arch/neon intrinsics still need to be adjusted (they are auto-generated and the generating tool needs some changes).

[bors]

☔ The latest upstream changes (presumably #85109) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

@bors rollup=iffy

[bors]

⌛ Testing commit bc2b62b1012d447e90fd5556cd84694593a987c7 with merge fcb1f66ecdd8af9435334dde4ceef8e1b2efd145...

[bors]

💔 Test failed - checks-actions

[RalfJung]

Needs rust-lang/stdarch#1162

[RalfJung]

@bors r=oli-obk

[bors]

📌 Commit c61e8fa has been approved by oli-obk

[bors]

⌛ Testing commit c61e8fa with merge d2df620...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing d2df620 to master...