Change codegen of LLVM intrinsics to be name-based, and add llvm linkage support for bf16(xN) and i1xN

[sayantn]

View all comments

This PR changes how LLVM intrinsics are codegen

Explanation of the changes

Current procedure

This is the same for all functions, LLVM intrinsics are not treated specially

• We get the LLVM Type of a function simply using the argument types. For example, the following function

  #[link_name = "llvm.sqrt.f32"]
  fn sqrtf32(a: f32) -> f32;

  will have LLVM type simply f32 (f32) due to the Rust signature

Pros

• Simpler to implement, no extra complexity involved due to LLVM intrinsics

Cons

• LLVM intrinsics have a well-defined signature, completely defined by their name (and if it is overloaded, the type parameters). So, this process of converting Rust signatures to LLVM signatures may not work, for example the following code generates LLVM IR without any problem

  #[link_name = "llvm.sqrt.f32"]
  fn sqrtf32(a: i32) -> f32;

  but the generated LLVM IR is invalid, because it has wrong signature for the intrinsic (Godbolt, adding -Zverify-llvm-ir to it will fail compilation). I would expect this code to not compile at all instead of generating invalid IR.
• LLVM intrinsics that have types in their signature that can't be accessed from Rust (notable examples are the AMX intrinsics that have the x86amx type, and (almost) all intrinsics that have vectors of i1 types) can't be linked to at all. This is a (major?) roadblock in the AMX and AVX512 support in stdarch.
• If code uses an non-existing LLVM intrinsic, even -Zverify-llvm-ir won't complain. Eventually it will error out due to the non-existing function (courtesy of the linker). I don't think this is a behavior we want.

What this PR does

• When linking to non-overloaded intrinsics, we use the function LLVMIntrinsicGetType to directly get the function type of the intrinsic from LLVM.
• We then use this LLVM definition to verify the Rust signature, and emit a proper error if it doesn't match, instead of silently emitting invalid IR.
• Lint if linking to deprecated or invalid LLVM intrinsics

Note

This PR only focuses on non-overloaded intrinsics, overloaded can be done in a future PR

Regardless, the undermentioned functionalities work for all intrinsics

• If we can't find the intrinsic, we check if it has been AutoUpgraded by LLVM. If not, that means it is an invalid intrinsic, and we error out.
• Don't allow intrinsics from other archs to be declared, e.g. error out if an AArch64 intrinsic is declared when we are compiling for x86

Pros

• It is now not possible (or at least, it would require significantly more leaps and bounds) to introduce invalid IR using non-overloaded LLVM intrinsics.
• As we are now doing the matching of Rust signatures to LLVM intrinsics ourselves, we can now add bypasses to enable linking to such non-Rust types (e.g. matching 8192-bit vectors to x86amx and injecting llvm.x86.cast.vector.to.tile and llvm.x86.cast.tile.to.vectors in callsite)

Note

I don't intend for these bypasses to be permanent. A better approach will be introducing a bf16 type in Rust, and allowing repr(simd) with bools to get Rust-native i1xNs. These are meant to be short-time, as I mentioned, "bypass"es. They shouldn't cause any major breakage even if removed, as link_llvm_intrinsics is perma-unstable.

This PR adds bypasses for bf16 (via i16), bf16xN (via i16xN) and i1xN (via iM, where M is the smallest power of 2 s.t. M >= N, unless N <= 4, where we use M = 8). This will unblock AVX512-VP2INTERSECT and a lot of bf16 intrinsics in stdarch. This PR also automatically destructures structs if the types don't exactly match (this is required for us to start emitting hard errors on mismmatches).

Cons

• This only works for non-overloaded intrinsics (at least for now). Improving this to work with overloaded intrinsics too will involve significantly more work.

Possible ways to extend this to overloaded intrinsics (future)

Parse the mangled intrinsic name to get the type parameters

LLVM has a stable mangling of intrinsic names with type parameters (in LLVMIntrinsicCopyOverloadedName2), so we can parse the name to get the type parameters, and then just do the same thing.

Pros

• For most intrinsics, this will work perfectly, and is a easy way to do this.

Cons

• The LLVM mangling is not perfectly reversible. When we have TargetExt types or identified structs, their name is a part of the mangling, making it impossible to reverse. Even more complexities arise when there are unnamed identified structs, as LLVM adds more mangling to the names.
• @nikic's work on LLVM intrinsics will remove the name mangling, making this approach impossible

Use the IITDescriptor table and the Rust function signature

We can use the base name to get the IITDescriptors of the corresponding intrinsic, and then manually implement the matching logic based on the Rust signature.

Pros

• Doesn't have the above mentioned limitation of the parsing approach, has correct behavior even when there are identified structs and TargetExt types. Also, fun fact, Rust exports all struct types as literal structs (unless it is emitting LLVM IR, then it always uses named identified structs, with mangled names)

Cons

• Doesn't actually use the type parameters in the name, only uses the base name and the Rust signature to get the llvm signature (although we can check that it is the correct name). It means there would be no way to (for example) link against llvm.sqrt.bf16 until we have bf16 types in Rust. Because if we are using u16s (or any other type) as bf16s, then the matcher will deduce that the signature is u16 (u16) not bf16 (bf16) (which would lead to an error because u16 is not a valid type parameter for llvm.sqrt), even though the intended type parameter is specified in the name.
• Much more complex, and hard to maintain as LLVM gets new IITDescriptorKinds

These 2 approaches might give different results for same function. Let's take

#[link_name = "llvm.is.constant.bf16"]
fn foo(a: u16) -> bool

The name-based approach will decide that the type parameter is bf16, and the LLVM signature is i1 (bf16) and will inject some bitcasts at callsite.
The IITDescriptor-based approach will decide that the LLVM signature is i1 (u16), and will see that the name given doesn't match the expected name (llvm.is.constant.u16), and will error out.

Reviews are welcome, as this is my first time actually contributing to rustc

@rustbot label T-compiler A-codegen A-LLVM
r? codegen

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[dianqk]

I think you can use LLVMGetIntrinsicDeclaration, LLVMGetIntrinsicDeclaration or some functions in Intrinsic.h in declare_raw_fn, as a reference: https://github.com/llvm/llvm-project/blob/d35ad58859c97521edab7b2eddfa9fe6838b9a5e/llvm/lib/AsmParser/LLParser.cpp#L330-L335.

[sayantn]

That can be used to improve performance, I am not really focusing on performance in this PR. I want to currently emphasize the correctness of the codegen.

[sayantn]

Oh wait, I probably misunderstood your comment, you meant using the llvm declaration by itself. Yeah, that would be better, thanks for the info. I will update the impl when I get the chance

[dianqk]

Oh wait, I probably misunderstood your comment, you meant using the llvm declaration by itself. Yeah, that would be better, thanks for the info. I will update the impl when I get the chance

I think you can just focus on non-overloaded functions for this PR. Overloaded functions and type checking that checking Rust function signatures using LLVM defined can be subsequent PRs.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[nikic]

@sayantn Taking the address of an intrinsic is invalid LLVM IR.

[dianqk]

@dianqk another small ping

I'm still a little busy, but I should be able to take this in this week whatever.

[dianqk]

The proposal makes sense to me. Great! I have a concern that rustc is supporting multiple LLVM versions. What about if we use an intrinsic that is deprecated in the new LLVM?

I also wonder whether it might make sense to implement these checks directly in LLVM rather than rustc. This can be beneficial to other languages. And we don't need to consider things like AutoUpgraded. If we'd like, we can provide a lint function that helps rustc show better diagnostics rather than verify-llvm-ir in LLVM.

[sayantn]

@dianqk thanks for the review ❤️

deprecated_llvm_intrinsic is a lint anyway (and it is disabled by default, because it is not incorrect to use a deprecated intrinsic, just it is better to switch to the new version), you can just disable it when you're using the older LLVM version. If you're talking about using intrinsics that are added in a later version, those will fail linking anyway, this PR will error at compile time instead, which is way easier to debug.

(In stdarch, we use the latest LLVM intrinsics, but that doesn't cause problems in the Rust CI, which uses the lowest supported LLVM version. This is because all the functions in stdarch are marked #[inline], so they aren't codegened. If we force codegening them, e.g. by -Clink-dead-code, these will fail compilation. This PR preserves this behavior, because all the checks are done only at codegen time)

IMO it doesn't make much sense to add these checks to LLVM. For once, LLVM already has the opt-in verify-ir feature which mostly covers this (except for the unknown intrinsic case, and the higher-quality error messages in this PR). Moreover, even if we added these to LLVM, we would still need something like this in the Rust frontend anyway to wrap the LLVM checks. Also, the original motive of the PR was to add bypasses for inaccessible types, which we also cannot do if we implement these checks in LLVM directly.

I didn't get your comment about AutoUpgrade, I am using AutoUpgrade to determine if it deprecated or unknown (because LLVMLookupIntrinsicID returns 0 in both cases).

[dianqk]

Can this function be moved in check_call?

[sayantn]

I don't think that is possible, because these autocasts etc should only be done for intrinsics. Also, I want to keep these changes as contained in intrinsics.rs as possible

[dianqk]

Should equate_ty be renamed to can_autocast or else? I don't think <2 x i1> equates to i8.

Do you think we could get rid of equate_ty(can_autocast)? Can we emit IntrinsicSignatureMismatch in autocast? This is not crucial. I have no clue which is better.

View changes since this review

[sayantn]

@dianqk thanks for the review, and sorry for the late reply.

Should equate_ty be renamed to can_autocast or else? I don't think <2 x i1> equates to i8.

No problems, idc about the names

Do you think we could get rid of equate_ty(can_autocast)? Can we emit IntrinsicSignatureMismatch in autocast? This is not crucial. I have no clue which is better.

Sorry, I don't get what you're trying to say here.

Here is a simpler version (url), I think:

Thanks a lot, it should work. I'll let you know

BTW, the zero-extended indices are a bit confused. For example, 0, 1, 2, 3, 4, 5, 3, 4 let me think why they are 3, 4, 5, 3, 4. Are they special magic numbers?

I just copied whatever LLVM does when it AutoUpgrades. I think your version should also work fine

[dianqk]

Do you think we could get rid of equate_ty(can_autocast)? Can we emit IntrinsicSignatureMismatch in autocast? This is not crucial. I have no clue which is better.

Sorry, I don't get what you're trying to say here.

Never mind. The current version is fine with me. I am just curious whether the IntrinsicSignatureMismatch check can be moved in autocast, so that we don't need to call assert!(equate_ty) and maintain the same check logic in both equate_ty and autocast.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[sayantn]

Oh, I see. The assert!(can_autocast) is just for my peace of mind. I can remove it or make it debug_assert! if you like.

[sayantn]

Here is a simpler version (url), I think:

Some updates on this, it worked perfectly, thanks a lot again! Moreover, I observed that the old zext shuffles produced code like (for i1x2 to i8 casts)

kshiftlw        k2, k0, 14
kshiftrw        k2, k2, 14
kmovw   eax, k2

which is what clang does too. But the new shuffles produce asm like

kmovw   eax, k0
and     al, 3

I have no idea which is faster, but they seem comparable

[dianqk]

I have no idea which is faster, but they seem comparable

Hmm, I have no idea either, but this can be changed back if this is an issue.

[dianqk]

LGTM.

View changes since this review

[dianqk]

@bors r+

[rust-bors]

📌 Commit 11f350d has been approved by dianqk

It is now in the queue for this repository.

[JonathanBrouwer]

@bors try jobs=i686-msvc-2

[rust-bors]

☀️ Try build successful (CI)
Build commit: 33337e3 (33337e3bc8ebd26bf0e5da091b2a79b9835326e1, parent: 14196dbfa3eb7c30195251eac092b1b86c8a2d84)