Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure out which target features are required for which SIMD size #131800

Open
14 of 16 tasks
RalfJung opened this issue Oct 16, 2024 · 53 comments
Open
14 of 16 tasks

Figure out which target features are required for which SIMD size #131800

RalfJung opened this issue Oct 16, 2024 · 53 comments
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-SIMD Area: SIMD (Single Instruction Multiple Data) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 16, 2024

The context for this is #116558: passing vector types by-value over extern "C" needs certain registers to be present, so if the target feature enabling these registers is missing, then either the ABI needs to change (which can lead to soundness issues if caller and callee disagree on their target features), or LLVM just errors outright.

#127731 moves us towards detecting this situation, but that approach needs data about which target feature is needed to pass which vector size. That will be different for each architecture. So this issue is about gathering all that data.

  • x86 (32bit and 64bit)
  • arm
    • "neon" | "mve" => vlen(128)
  • aarch64:
  • powerpc
    • "altivec" => vlen(128)
    • "mma" => vlen(512), NOTE: not supported by clang
  • riscv
    • "zve32x" | "zve32f" => vlen(32),
    • "zve64x" | "zve64f" | "zve64d" => vlen(64),
    • "zvl128b" | "v" => vlen(128)
    • "zvl{N}b" => "it's complicated"
  • wasm: "simd128" => vlen(128)
  • loongarch: should have an on-stack ABI
  • s390x: "vector" => vlen(128)
  • sparc: "vis" => vlen(64) (or more? hard to say)
  • hexagon
    • "hvx-length64b" => vlen(512)
    • "hvx-length128b" => vlen(1024)
  • mips and mips*r6: "msa" => vlen(128)
  • nvptx: vlen(128)
  • bpf: none
  • csky: "vdspv1" | "vdspv2" => vlen(128),
  • m68k
  • more?
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 16, 2024
@lolbinarycat lolbinarycat added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-SIMD Area: SIMD (Single Instruction Multiple Data) A-ABI Area: Concerning the application binary interface (ABI) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 16, 2024
@workingjubilee
Copy link
Member

cc @programmerjake to confirm what features are relevant re: PowerPC

@workingjubilee
Copy link
Member

workingjubilee commented Oct 17, 2024

@programmerjake
Copy link
Member

+altivec enables 128-bit vectors, I'm not sure if there are any wider types -- there's MMA with 512-bit accumulators, but idk if they are vector types, they're used for matrix ops.

@topperc
Copy link

topperc commented Oct 17, 2024

riscv: cc @kito-cheng or @topperc to confirm for LLVM features that can affect the vector ABI?

+zve32x, +zve32f, +zve64x, +zve64f, +zve64d, +v, +zvbb, +zvkb. There several others that start with +zv*. There is an implies relationship so they all ultimately imply +zve32x. These change the ABI for fixed length vector arguments/returns in IR in the backend.

When compiling with clang with the default C ABI, fixed length vectors are passed coerced to a scalar integer type or passed indirectly through memory. clang will not create fixed length vector return types or arguments in IR.

There is a fixed length vector ABI being implemented via an attribute llvm/llvm-project#100346. This changes how fixed length vectors are passed.

@jieyouxu jieyouxu added E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Oct 17, 2024
@programmerjake
Copy link
Member

vsx just enables an additional 32 registers that overlap with the scalar floating point registers but are otherwise the same as the 32 128-bit registers from vmx (aka. altivec), so no new simd bitwidths.

@workingjubilee
Copy link
Member

@programmerjake and no alterations to the calling convention?

@workingjubilee
Copy link
Member

@topperc hm, it seems LLVM doesn't have the crypto implications for these features specified here? is it somewhere else? https://github.com/llvm/llvm-project/blob/d54953ef472bfd8d4b503aae7682aa76c49f8cc0/llvm/lib/Target/RISCV/RISCVFeatures.td#L734-L746

it seems to rather be the opposite, a requirement relationship, but perhaps I'm misunderstanding: https://github.com/llvm/llvm-project/blob/d54953ef472bfd8d4b503aae7682aa76c49f8cc0/llvm/lib/TargetParser/RISCVISAInfo.cpp#L754-L758

@topperc
Copy link

topperc commented Oct 17, 2024

@topperc hm, it seems LLVM doesn't have the crypto implications for these features specified here? is it somewhere else? https://github.com/llvm/llvm-project/blob/d54953ef472bfd8d4b503aae7682aa76c49f8cc0/llvm/lib/Target/RISCV/RISCVFeatures.td#L734-L746

it seems to rather be the opposite, a requirement relationship, but perhaps I'm misunderstanding: https://github.com/llvm/llvm-project/blob/d54953ef472bfd8d4b503aae7682aa76c49f8cc0/llvm/lib/TargetParser/RISCVISAInfo.cpp#L754-L758

My mistake. I'm not sure why we don't have the implies. gcc does.

@programmerjake
Copy link
Member

@programmerjake and no alterations to the calling convention?

enabling vsx doesn't alter the calling convention.

@RalfJung
Copy link
Member Author

+altivec enables 128-bit vectors, I'm not sure if there are any wider types -- there's MMA with 512-bit accumulators, but idk if they are vector types, they're used for matrix ops.

Are there types which are passed via these MMA registers?

@RalfJung
Copy link
Member Author

RalfJung commented Oct 17, 2024

My mistake. I'm not sure why we don't have the implies. gcc does.

For us it's nice that there's no "implies" here, that makes it a lot easier to check the ABI consequences. ;) This way we juts have to block the actual feature changing something, not other features implying them.

Though maybe this is also unnecessary if #131807 takes care of all that.

EDIT: Ah, that's just for float ABI, not for vectors, is it?

@RalfJung
Copy link
Member Author

RalfJung commented Oct 17, 2024

-Cllvm-args="--riscv-v-vector-bits-min=N"

Uh wait a second, we are exposing another flag that can change ABI? 😢 😭

EDIT: That's probably a discussion for Zulip.

@heiher
Copy link
Contributor

heiher commented Oct 17, 2024

LoongArch: According to the LoongArch ABI Specs, vector type parameters and return values ​​are passed in GAR(general-purpose argument registers) or on the stack, and do not rely on vector registers or vector features.

@programmerjake
Copy link
Member

programmerjake commented Oct 17, 2024

+altivec enables 128-bit vectors, I'm not sure if there are any wider types -- there's MMA with 512-bit accumulators, but idk if they are vector types, they're used for matrix ops.

Are there types which are passed via these MMA registers?

after a bit more research, there are types for MMA, but you can't pass them by value in function arguments or return, so they're not ABI-breaking: https://clang.godbolt.org/z/e4sTY37Pv
they lower to <256 x i1> and <512 x i1>

@workingjubilee
Copy link
Member

...concerning. these blockades are in clang's semantic checks, they don't seem to be enforced by LLVM.

@workingjubilee
Copy link
Member

cc @jacobbramley re: aarch64

@workingjubilee
Copy link
Member

cc @androm3da re: hexagon

@RalfJung
Copy link
Member Author

after a bit more research, there are types for MMA, but you can't pass them by value in function arguments or return

How do we handle that in Rust? We'd need a special pass during collection rejecting them as arguments, likely as part of the simd arg check that this issue is about.

I assume we don't support these types yet, but this will need to be considered when someone decides to add them.

@RalfJung
Copy link
Member Author

-Cllvm-args="--riscv-v-vector-bits-min=N"

Uh wait a second, we are exposing another flag that can change ABI? 😢 😭

I only just realized this only affects scalable vector types. Which anyway we don't support. So we can ignore this for now.

@Dirreke
Copy link
Contributor

Dirreke commented Oct 17, 2024

According to the CSKY Development Guide: 4.5 vdsp, the vector width is configured as follows:

For vdspv2, the width is fixed at 128 bits.
For vdspv1, the default width is 128 bits, but it can optionally be set to 64 bits using the -mvdsp-width=64 compiler flag. However, please note that the 64-bit width option is currently unsupported by LLVM.

Therefore, for both versions, you can safely set the vector width to 128 bits.

csky:
"vdspv1" | "vdspv2" => vlen(128),

veluca93 added a commit to veluca93/rust that referenced this issue Nov 10, 2024
See rust-lang#131800 for the data collection behind this change.
@thejpster
Copy link
Contributor

The SPARC systems I look after (V7 and V8) don't have any vector extensions.

veluca93 added a commit to veluca93/rust that referenced this issue Nov 10, 2024
See rust-lang#131800 for the data collection behind this change.
@RalfJung
Copy link
Member Author

RalfJung commented Nov 10, 2024

Seems like the answer is vis (according to @taiki-e )

@RalfJung
Copy link
Member Author

Regarding RISC-V, it seems that right now our ABI adjustment logic will never use LLVM vector types for argument passing. It will make them indirect or use an integer register (if the vector is small enough). So I guess the plan is that that logic is updated if/when we get support for the corresponding target features? That will mean we are breaking ABI for any existing code that passes repr(simd) types by-val. However, that's only possible on nightly, so I guess this is fine?

Still, it'd probably be nicer if the ABI adjustment logic just said "vector types currently not supported for this architecture" instead of computing some non-standard ABI.

veluca93 added a commit to veluca93/rust that referenced this issue Nov 10, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
veluca93 added a commit to veluca93/rust that referenced this issue Nov 10, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
veluca93 added a commit to veluca93/rust that referenced this issue Nov 10, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
veluca93 added a commit to veluca93/rust that referenced this issue Nov 10, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
veluca93 added a commit to veluca93/rust that referenced this issue Nov 10, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
veluca93 added a commit to veluca93/rust that referenced this issue Nov 10, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
veluca93 added a commit to veluca93/rust that referenced this issue Nov 11, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
veluca93 added a commit to veluca93/rust that referenced this issue Nov 12, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
veluca93 added a commit to veluca93/rust that referenced this issue Nov 12, 2024
See rust-lang#131800 for the data collection behind this change.

Also adds a test that exercise the "empty list of features" path.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 13, 2024
…ngjubilee

ABI checks: add support for tier2 arches

See rust-lang#131800 for the data collection behind this change.

r? RalfJung
@RalfJung
Copy link
Member Author

How big are the SPARC vectors that get unlocked by the vis feature?

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 13, 2024
…ngjubilee

ABI checks: add support for tier2 arches

See rust-lang#131800 for the data collection behind this change.

r? RalfJung
@taiki-e
Copy link
Member

taiki-e commented Nov 13, 2024

How big are the SPARC vectors that get unlocked by the vis feature?

AFAIK it's at least 64-bit.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 13, 2024

@veluca93's PR has a comment indicating 128 now, that seems wrong then?
64bit vector registers seem kind of odd on a 64bit system though...

On SPARC64: 128-bit(arg)/256-bit(ret) or smaller integer/float vectors are passed using FP reg

That doesn't mean the register is 256 bits large though, is it? It probably uses more than one register? OTOH vectors larger than a register are also odd. So I don't really understand what to do with this.

@taiki-e
Copy link
Member

taiki-e commented Nov 13, 2024

SPARC FP registers (f[0-63]) are 32-bit long, and two/four of these are combined to process f64/f128. 64-bit VIS vectors also use two FP registers, as does f64.

128-bit/258-bit vectors are also passed or returned using four/eight FP registers.
https://github.com/gcc-mirror/gcc/blob/730f28b081bea4a749f9b82902446731ec8faa93/gcc/config/sparc/sparc.cc#L7388

(By the way, this complication of the nature of FP registers is one of the reasons I postponed FP register support in the initial implementation of SPARC inline assembly.)

@RalfJung
Copy link
Member Author

RalfJung commented Nov 13, 2024

The relevant question here is: for a vector of size N bits, which target feature must be present so that the vector is passed in a register (whereas if the feature is absent, vectors of that size are passed in a different way). Or I guess in this case, in a group of registers, or whatever these are called.

@taiki-e
Copy link
Member

taiki-e commented Nov 13, 2024

IIUC, in that case, we need to refer to the calling conventions'.

  • According to calling conventions implemented by GCC:
    • On SPARC32: 64-bit or smaller integer vectors are passed using FP reg
    • On SPARC64: 128-bit(arg)/256-bit(ret) or smaller integer/float vectors are passed using FP reg

Assuming our ABI code is correct:

  • "sparc": "vis" => vlen(64)
  • "sparc64": "vis" => vlen(256)

However, IIUC our ABI code does not support vector types correctly, so for now we may need to treat it as vlen(0) for SPARC32 and vlen(128) for SPARC64 (EDIT: see #131800 (comment)) in the lint. (Otherwise, it could incorrectly used the SPARC64 256-bit vectors in arguments or the SPARC32 float vectors. )

@RalfJung
Copy link
Member Author

You said it's 256 only for return values... not sure how that makes any sense, but something can't be right here then. It should probably be "128" for sparc64? Like, maybe in the future they define 256 bit vectors and then pass them different for arguments. Or so?

@taiki-e
Copy link
Member

taiki-e commented Nov 13, 2024

You said it's 256 only for return values... not sure how that makes any sense, but something can't be right here then.

The 256-bit vector in argument position is passed through memory, not FP registers.

From calling conventions implemented by GCC:

                           size      argument     return value
 vector integer           <=16        FP reg.        FP reg.
 vector integer       16<s<=32        memory         FP reg.
 vector integer            >32        memory         memory

IIUC, our ABI code should handle this correctly by calling make_indirect for vectors with size>16(argument) or size>32(return value). (If it is handled correctly, the 256-bit vector in argument position should not generate a warning anyway, since the lint trigger condition is no longer satisfied.)

maybe in the future they define 256 bit vectors

GCC's VIS builtins support 32-bit and 64-bit vectors, but larger sizes of vector types are available via vector_size(...). (In Rust, the former is core::arch vector type (not available for SPARC yet) and the latter is core::simd or custom #[repr(simd)] vector type)

Here is a code generated by GCC (only 256-bit vector is moved from memory to FP reg): https://godbolt.org/z/5vEdejj6j

@taiki-e
Copy link
Member

taiki-e commented Nov 13, 2024

In any case, LLVM doesn't currently support Vector ABI (llvm/llvm-project#45418), so it seems that using vlen(0) in the lint is correct for now.

@RalfJung
Copy link
Member Author

I guess the number we need is "for vectors of which size is this calling convention guaranteed to be the final word, and future extensions won't change how those vectors are passed". Even assuming a correct ABI implementation on our side, what is that number for SPARC32 + vis / SPARC64 + vis? Or is this somehow not a well-formed question?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 13, 2024
…ngjubilee

ABI checks: add support for tier2 arches

See rust-lang#131800 for the data collection behind this change.

r? RalfJung
@taiki-e
Copy link
Member

taiki-e commented Nov 14, 2024

SPARC's Vector ABI is defined based on the existing float and aggregate calling convention, not the VIS ISA 1, and changing it without a new ABI would also break other non-vector arguments due to the nature of using FP registers. So, I don't believe it can be changed without a new ABI. (This is very different from the x86_64, which extended the ISA in the form of increasing the size of the vector registers.)

That said, if our policy is to not trust the current behavior of psABI with respect to vectors larger than ISA supports 2, I think the 64-bit mentioned in #131800 (comment) is safe selection.

Footnotes

  1. sparc_pass_by_reference and sparc_return_in_memory in GCC have comments about this.

  2. I think this is the correct policy for most architectures.

rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 14, 2024
Rollup merge of rust-lang#132842 - veluca93:abi-checks-tier2, r=workingjubilee

ABI checks: add support for tier2 arches

See rust-lang#131800 for the data collection behind this change.

r? RalfJung
@RalfJung
Copy link
Member Author

SPARC's Vector ABI is defined based on the existing float and aggregate calling convention, not the VIS ISA

So having or not having vis actually makes no difference for how things are passed?
That would mean we could actually allow arbitrary-size vectors -- once we implement the existing ABI correctly, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-SIMD Area: SIMD (Single Instruction Multiple Data) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests