manually const-ify shuffle arguments

[RalfJung]

This means we do not rely on promotion any more for these arguments. Prerequisite for rust-lang/rust#85110.

However, I could not figure out what is generating these calls:

stdarch/crates/core_arch/src/aarch64/neon/generated.rs

Line 1174 in 4cc8cf6

0 => simd_shuffle8(a, b, [8 + LANE2 as u32, 1, 2, 3, 4, 5, 6, 7]),

The correspond "spec" seems to be

/// Insert vector element from another vector element
name = vcopy
lane-suffixes
constn = LANE1:LANE2
multi_fn = static_assert_imm-in0_exp_len-LANE1
multi_fn = static_assert_imm-in_exp_len-LANE2
multi_fn = matchn-in0_exp_len-LANE1, simd_shuffle-out_len-noext, a, b, {ins-in0_len-in0_len-LANE2}
a = 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16
b = 0, MAX, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
n = 0:1
validate MAX, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16

But I couldn't figure out how this would have to be modified to generate calls to simd_shuffleN_param! here.

[rust-highfive]

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

[RalfJung]

Funny that it complains about style, in some files when I did rustfmt it caused changes in code I had not touched so I figured I'd not run rustfmt here...

[RalfJung]

Yeah, this is also the case in avx512f.rs, which it now complains about. I am not sure how I should properly format that code.
(I am doing this all directly in the submodule in rustc so that I can build-test it against a compiler that includes rust-lang/rust#85110; this might affect rustfmt behavior.)

[Amanieu]

@SparrowLii Can you help with the modifications needed to stdarch-gen?

[Amanieu]

For rustfmt, I think it's because it is inheriting the rustfmt.toml of rustc. Try creating an empty rustfmt.toml in the stdarch root.

[RalfJung]

That didn't help; I ended up checking out the branch in a separate clone of the repo and doing the fmt there.

[SparrowLii]

But I couldn't figure out how this would have to be modified to generate calls to simd_shuffleN_param! here.

I will give the corresponding changes.

[RalfJung]

I think using statements like multi_fn = simd_shuffle-out_len-_param! will be fine.

I don't think that will work, there also needs to be a <const LANE2: i32> added right before the array:

simd_shuffle8_param!(a, b, <const LANE2: i32> [8 + LANE2 as u32, 1, 2, 3, 4, 5, 6, 7]),

We could change the macro syntax if that helps, but the macro does need to explicitly know name and type of const parameters that the array expression depends on. (Currently only a single param is supported, but I can easily improve the macro.)

[RalfJung]

Anyway this PR is green now. :D We don't have to land all these changes in one go, so if @SparrowLii is taking care of the remaining neon issues, that could happen in a separate PR and I think this PR could land already?

[SparrowLii]

You just need change this line to the following:

    let mut s = if fn_format[0].ends_with(":const") {
        format!("<const {}: i32> [", fn_format[3])
    } else {
        String::from("[")
    };

Then use multi_fn = matchn-in0_exp_len-LANE1, simd_shuffle-out_len-_param!, a, b, {ins:const-in0_len-in0_len-LANE2}

[RalfJung]

@SparrowLii that did not entirely work, but doing this instead helped:

let mut s = format!("<const {}: i32> [", fn_format[3]);

I made the macros a bit easier to use so the same macro supports arrays with and without use of constants.

However, the generated file still contains some incorrect functions like

pub unsafe fn vdupq_laneq_p64<const N: i32>(a: poly64x2_t) -> poly64x2_t {
    static_assert_imm1!(N);
    simd_shuffle2!(a, a, [N as u32, N as u32])
}

Here a <const N: i32> is missing before the array.

[RalfJung]

I think what should work is to simply add all constn before each of the shuffle arrays, but get_call does not seem to have access to that information.

[RalfJung]

I think I found a way to forward that information to get_call... let's see what CI has to say about this.

[RalfJung]

Ah CI is looking good. :)

[RalfJung]

The one disadvantage of this approach is that it unnecessarily adds LANE1 to the generics of the constant.

[SparrowLii]

We cannot ensure that the constn_declare required by simd_shuffle*! is consistent with the constn_declare in the function signature, so I think it might be better to use ins:constn and dup:constn in the spec.

[SparrowLii]

Or if you can allow a comma between <constn LANE2: i32> and the square brackets,(like simd_shuffle8_param!(a, b, <const LANE2: i32>, [8 + LANE2 as u32, 1, 2, 3, 4, 5, 6, 7]) ) we can simply add another parameter after a, b in spec

[RalfJung]

We cannot ensure that the constn_declare required by simd_shuffle*! is consistent with the constn_declare in the function signature

Well, the simd_shuffle only requires any superset of the constants to be declared. I am not sure what the cost of declaring "too much" is, it would mean the associated const has more monomorphizations -- but since the surrounding function already has that number of monomorphizations, I doubt that is a big problem.

IOW, I think the code generated currently is fine, and this can always be improved if it does turn out to be a problem.

Or if you can allow a comma

That sounds a bit hacky but I think allowing a comma is probably easy.

[SparrowLii]

That sounds a bit hacky but I think allowing a comma is probably easy.

Just forget it plz. That will cause other problems (because it contains : )