Add 2048-bit HvxVectorPair support to Hexagon SIMD ABI checks

[androm3da]

Previously, rustc rejected HvxVectorPair types in function signatures because the HEXAGON_FEATURES_FOR_CORRECT_FIXED_LENGTH_VECTOR_ABI array only had entries for vectors up to 1024 bits. This caused the ABI checker to emit "unsupported vector type" errors for 2048-bit HvxVectorPair (used in 128-byte HVX mode).

Add 2048 byte entry to allow HvxVectorPair to be passed in extern "C" funcs when the hvx-length128b is enabled.

[rustbot]

r? @madsmtm

rustbot has assigned @madsmtm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 68 candidates
• Random selection from 17 candidates

[madsmtm]

Am I correctly understanding that a 1024-bit HvxVectorPair would actually require the hvx-length64b feature (and that the check is overly conservative here)?

[madsmtm]

It actually sounds like the real problem is the check itself, features_for_correct_fixed_length_vector_abi changes depending on PassMode::Pair or PassMode::Direct?

[androm3da]

It actually sounds like the real problem is the check itself, features_for_correct_fixed_length_vector_abi changes depending on PassMode::Pair or PassMode::Direct?

Hmm good point let me take a closer look.

[androm3da]

I could do this...

const HEXAGON_FEATURES_FOR_CORRECT_FIXED_LENGTH_VECTOR_ABI: &'static [(u64, &'static str)] = &[
    (512, "hvx-length64b"),
    (1024, "hvx"),           // hvx is implied by both length modes
    (2048, "hvx-length128b"),
];

... I don't love it though. Not the clearest error message.

I wonder if I could specialize like so

fn check_vector_abi_feature(&self, size_bits: u64, have_feature: impl Fn(&str) -> bool) -> Result<(), 
&'static str> {
    match size_bits {
        0..=512 => {
            if have_feature("hvx-length64b") { Ok(()) }
            else { Err("hvx-length64b") }
        }
        513..=1024 => {
            if have_feature("hvx-length64b") || have_feature("hvx-length128b") { Ok(()) }
            else { Err("hvx-length64b or hvx-length128b") }
        }
        1025..=2048 => {
            if have_feature("hvx-length128b") { Ok(()) }
            else { Err("hvx-length128b") }
        }
        _ => Err("unsupported vector size")
    }
}

[madsmtm]

Hmm, I'm not sure. I think your proposed solution would accept in a sense "too many" ABIs, would it not? E.g. passing a 1024-bit HvxVector with only the "hvx-length64b" feature enabled should fail in a correct implementation (if I'm understanding right)?

---

Kinda odd though that all of this isn't a problem on other architectures, do you understand why that is? Is there something special about Hexagon's pair vectors and feature flags here?

[androm3da]

I think other architectures' target features are cumulative - +newest-whizbang implies +slightly-older-whizbang and that in turn implies +older-still-whizbang. HVX 64 byte mode and 128 byte mode are mutually exclusive. So now we have ambiguity if we're trying to map a size to a feature. 1024 bit values could either be a single 128 byte register in hvx-length128b and a 128-byte regpair in hvx-length64b. Both of these imply hvx but none of these imply the other lengths.

I don't know how the PassMode works yet - I don't think the HvxVectorPair would use PassMode::Pair but I will do some digging to try and figure it out.

[madsmtm]

Ah, I didn't get that they were incompatible. Must at least one of them be set? If so, then I'd lean towards (1024, "hvx"),.

I don't think the HvxVectorPair would use PassMode::Pair but I will do some digging to try and figure it out.

Oh, okay.

[madsmtm]

Not familiar with Hexagon, but this looks correct enough, and you're the target maintainer, so:
@bors r+ rollup

[rust-bors]

📌 Commit eb1e411 has been approved by madsmtm

It is now in the queue for this repository.