Codegen Overloaded LLVM intrinsics based on their name - #157145
Conversation
This comment has been minimized.
This comment has been minimized.
a399a07 to
c55c137
Compare
This comment has been minimized.
This comment has been minimized.
c55c137 to
36e3bbc
Compare
|
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. |
|
cc @rust-lang/wg-llvm could you have a look here - when you have some time? thanks |
I will review this PR this week. |
|
@bors r+ |
…ianqk Codegen Overloaded LLVM intrinsics based on their name This is a continuation of rust-lang#140763 - now codegenning overloaded LLVM intrinsics based on their name too. This PR parses the `link_name` of the LLVM intrinsics for the type parameters, partially inverting [`getMangledTypeStr`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00076) and [`getIntrinsicNameImpl`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00165) from LLVM. There is the concern that @nikic's work on LLVM intrinsics might remove the name mangling, but we can just retain that from the Rust side. I mean even though the LLVM IR wouldn't have the mangling, but we can require that the Rust `link_name` argument contain the mangling. This shouldn't break anything, as existing code already has the name mangling. There is also the concern that this cannot parse `TargetExt` types and non-literal struct types, as their mangling contains their name. If needed in future, we can maybe hardcode some known `TargetExt` types, but currently we don't support it. It also kinda helps that Rust currently cannot handle `TargetExt` types. The named struct one is not that big of a problem because courtesy of rust-lang#140763 we can already repack structs. I have not added support of LLVM `byte` type because it is only available in LLVM22, and we support min-LLVM version 20 afaik. I prefer this approach over the `IITDesc` approach highlighted in rust-lang#140763 because this approach allows code like ```rust #[link_name = "llvm.sqrt.v8bf16"] fn foo(a: u16x8) -> u16x8; ``` which pairs up with the autocasts of rust-lang#140763 to give a nice way to call overloaded intrinsics on `bf16`. Also this approach is a lot less work and more resilient to LLVM changes than the `IITDesc` approach. One important change - the parsing doesn't account for LLVM typed pointers, which were deprecated in LLVM15 and removed in LLVM17, so I didn't bother putting support for them. So, I also removed all uses of typed pointers from the tree. r? @dianqk as you might have more of a context on this due to reviewing the last 2 PRs cc @nikic
…ianqk Codegen Overloaded LLVM intrinsics based on their name This is a continuation of rust-lang#140763 - now codegenning overloaded LLVM intrinsics based on their name too. This PR parses the `link_name` of the LLVM intrinsics for the type parameters, partially inverting [`getMangledTypeStr`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00076) and [`getIntrinsicNameImpl`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00165) from LLVM. There is the concern that @nikic's work on LLVM intrinsics might remove the name mangling, but we can just retain that from the Rust side. I mean even though the LLVM IR wouldn't have the mangling, but we can require that the Rust `link_name` argument contain the mangling. This shouldn't break anything, as existing code already has the name mangling. There is also the concern that this cannot parse `TargetExt` types and non-literal struct types, as their mangling contains their name. If needed in future, we can maybe hardcode some known `TargetExt` types, but currently we don't support it. It also kinda helps that Rust currently cannot handle `TargetExt` types. The named struct one is not that big of a problem because courtesy of rust-lang#140763 we can already repack structs. I have not added support of LLVM `byte` type because it is only available in LLVM22, and we support min-LLVM version 20 afaik. I prefer this approach over the `IITDesc` approach highlighted in rust-lang#140763 because this approach allows code like ```rust #[link_name = "llvm.sqrt.v8bf16"] fn foo(a: u16x8) -> u16x8; ``` which pairs up with the autocasts of rust-lang#140763 to give a nice way to call overloaded intrinsics on `bf16`. Also this approach is a lot less work and more resilient to LLVM changes than the `IITDesc` approach. One important change - the parsing doesn't account for LLVM typed pointers, which were deprecated in LLVM15 and removed in LLVM17, so I didn't bother putting support for them. So, I also removed all uses of typed pointers from the tree. r? @dianqk as you might have more of a context on this due to reviewing the last 2 PRs cc @nikic
…ianqk Codegen Overloaded LLVM intrinsics based on their name This is a continuation of rust-lang#140763 - now codegenning overloaded LLVM intrinsics based on their name too. This PR parses the `link_name` of the LLVM intrinsics for the type parameters, partially inverting [`getMangledTypeStr`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00076) and [`getIntrinsicNameImpl`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00165) from LLVM. There is the concern that @nikic's work on LLVM intrinsics might remove the name mangling, but we can just retain that from the Rust side. I mean even though the LLVM IR wouldn't have the mangling, but we can require that the Rust `link_name` argument contain the mangling. This shouldn't break anything, as existing code already has the name mangling. There is also the concern that this cannot parse `TargetExt` types and non-literal struct types, as their mangling contains their name. If needed in future, we can maybe hardcode some known `TargetExt` types, but currently we don't support it. It also kinda helps that Rust currently cannot handle `TargetExt` types. The named struct one is not that big of a problem because courtesy of rust-lang#140763 we can already repack structs. I have not added support of LLVM `byte` type because it is only available in LLVM22, and we support min-LLVM version 20 afaik. I prefer this approach over the `IITDesc` approach highlighted in rust-lang#140763 because this approach allows code like ```rust #[link_name = "llvm.sqrt.v8bf16"] fn foo(a: u16x8) -> u16x8; ``` which pairs up with the autocasts of rust-lang#140763 to give a nice way to call overloaded intrinsics on `bf16`. Also this approach is a lot less work and more resilient to LLVM changes than the `IITDesc` approach. One important change - the parsing doesn't account for LLVM typed pointers, which were deprecated in LLVM15 and removed in LLVM17, so I didn't bother putting support for them. So, I also removed all uses of typed pointers from the tree. r? @dianqk as you might have more of a context on this due to reviewing the last 2 PRs cc @nikic
…ianqk Codegen Overloaded LLVM intrinsics based on their name This is a continuation of rust-lang#140763 - now codegenning overloaded LLVM intrinsics based on their name too. This PR parses the `link_name` of the LLVM intrinsics for the type parameters, partially inverting [`getMangledTypeStr`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00076) and [`getIntrinsicNameImpl`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00165) from LLVM. There is the concern that @nikic's work on LLVM intrinsics might remove the name mangling, but we can just retain that from the Rust side. I mean even though the LLVM IR wouldn't have the mangling, but we can require that the Rust `link_name` argument contain the mangling. This shouldn't break anything, as existing code already has the name mangling. There is also the concern that this cannot parse `TargetExt` types and non-literal struct types, as their mangling contains their name. If needed in future, we can maybe hardcode some known `TargetExt` types, but currently we don't support it. It also kinda helps that Rust currently cannot handle `TargetExt` types. The named struct one is not that big of a problem because courtesy of rust-lang#140763 we can already repack structs. I have not added support of LLVM `byte` type because it is only available in LLVM22, and we support min-LLVM version 20 afaik. I prefer this approach over the `IITDesc` approach highlighted in rust-lang#140763 because this approach allows code like ```rust #[link_name = "llvm.sqrt.v8bf16"] fn foo(a: u16x8) -> u16x8; ``` which pairs up with the autocasts of rust-lang#140763 to give a nice way to call overloaded intrinsics on `bf16`. Also this approach is a lot less work and more resilient to LLVM changes than the `IITDesc` approach. One important change - the parsing doesn't account for LLVM typed pointers, which were deprecated in LLVM15 and removed in LLVM17, so I didn't bother putting support for them. So, I also removed all uses of typed pointers from the tree. r? @dianqk as you might have more of a context on this due to reviewing the last 2 PRs cc @nikic
| pub unsafe fn overloaded_bf16_autocast(a: i16x8) -> i16x8 { | ||
| extern "unadjusted" { | ||
| #[link_name = "llvm.sqrt.v8bf16"] | ||
| fn foo(a: i16x8) -> i16x8; |
There was a problem hiding this comment.
I find this extremely confusing. What is going on here? The intrinsic actually has type <8 x bfloat> but we pass it an i16x8? Why should we allow such code...?
There was a problem hiding this comment.
I think the main motivation for this entire approach was ability to insert magic type casts for types that Rust does not support.
Of course, this is kind of moot with f16b being introduced in #160859.
(FWIW, I don't think what this PR does is a good idea, but I don't have time to fight it.)
There was a problem hiding this comment.
Of course, this is kind of moot with f16b being introduced in #160859.
Yeah that seems to remove this part of the motivation. OTOH the old referenced PR also mentions
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.
So the question is, will we have native types for those as well? We might have to, if they are relevant for ABIs we need to implement. But if they are only used for LLVM intrinsics, not for extern "C", then this PR may save us from adding that type. I don't know anything about x86amx, or whether there's a better way to deal with vectors of i1.
Another motivation seems to be dealing with LLVM intrinsics that change their signature -- how is stdarch supposed to import these? But this does works today so it seems the LLVM auto-upgrade for this is enough?
And then there's "finding signatures that are just wrong". But it seems that already landed in #140763.
On the list of downsides we have "keeping the long mangled LLVM names even after LLVM itself doesn't use them any more", if I understood correctly the plan for how to deal with @nikic's work on getting rid of the name mangling in LLVM. That seems like a silly end state and is clearly path-dependent -- if LLVM had removed the mangling 5 years ago, I doubt we'd accept a PR like this to add magic name mangling in the link_name string to indicate the "real" signature. So likewise I don't think we want to be in a state where we are keeping the name mangling alive long after LLVM dropped it.
I also couldn't find an MCP for this change. Should it have one? It's very backend-internal, but OTOH one of our main backend experts disagrees so it may be good to have a bit of a wider discussion. @dianqk seems fine with it.
There was a problem hiding this comment.
The intrinsics name mangling has been in Rust for a long time, before #140763. IMO, this PR just helps handwriting intrinsics for people. This is fine with me.
I think we may want to find an approach to get rid of the name mangling after LLVM changed, even if we are not going to merge the PR.
But I'm curious about alternative implementation. Can we write a cargo out-of-tree plugin for the intrinsics name mangling? IIUC, since this is mostly used for stdarch, a standalone checking tool would be great.
There was a problem hiding this comment.
The intrinsics name mangling has been in Rust for a long time, before #140763
demangle_type_str is new in this PR, so I am not sure what you mean. So far these mangled names are just forwarded opaquely to LLVM.
There was a problem hiding this comment.
To clarify my position on this: I consider name mangling to be something of an implementation detail of LLVM, that Rust would ideally not need to expose. Instead of declaring something like #[link_name = "llvm.sqrt.v8bf16"], it is sufficient to use #[link_name = "llvm.sqrt"] and infer the mangled name from the function signature. This is how intrinsics are written in textual LLVM IR nowadays, you don't actually write out the mangling suffix, the parser infers it.
The motivation for inverting this, and instead make the mangling suffix (rather than the function signature) the source of truth is ability to insert automatic casts for types that Rust does not support. This is no longer relevant for bfloat, but still relevant for x86_amx. IMHO the proper way to handle that is to represent this as a special type in Rust, which has special ABI rules when passed to LLVM intrinsics. This does not have to be full support for the type like what is happening with f16b right now, but just the minimum to adjust its ABI (which I assume is something like a lang item?)
There was a problem hiding this comment.
IMHO the proper way to handle that is to represent this as a special type in Rust, which has special ABI rules when passed to LLVM intrinsics. This does not have to be full support for the type like what is happening with f16b right now, but just the minimum to adjust its ABI (which I assume is something like a lang item?)
rust-lang/rfcs#4003 does this for two more types.
For types that don't get publicly exposed but only used internally, I think this doesn't need an RFC. Just a t-compiler MCP.
Rollup of 7 pull requests Successful merges: - #150075 (Implement clamp_to) - #157145 (Codegen Overloaded LLVM intrinsics based on their name) - #161866 (delegation: add tests fixating behavior of delegating to default trait implementations) - #161456 (reduce perf impact of scalar size checks) - #161730 (Improve type mismatch annotation for lets with block-wrapped initializers) - #161828 (Never type after-stabilization cleanup) - #161860 (atomicptr.rs test: remove unused import)
…ianqk Codegen Overloaded LLVM intrinsics based on their name This is a continuation of rust-lang#140763 - now codegenning overloaded LLVM intrinsics based on their name too. This PR parses the `link_name` of the LLVM intrinsics for the type parameters, partially inverting [`getMangledTypeStr`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00076) and [`getIntrinsicNameImpl`](https://llvm.org/doxygen/Intrinsics_8cpp_source.html#l00165) from LLVM. There is the concern that @nikic's work on LLVM intrinsics might remove the name mangling, but we can just retain that from the Rust side. I mean even though the LLVM IR wouldn't have the mangling, but we can require that the Rust `link_name` argument contain the mangling. This shouldn't break anything, as existing code already has the name mangling. There is also the concern that this cannot parse `TargetExt` types and non-literal struct types, as their mangling contains their name. If needed in future, we can maybe hardcode some known `TargetExt` types, but currently we don't support it. It also kinda helps that Rust currently cannot handle `TargetExt` types. The named struct one is not that big of a problem because courtesy of rust-lang#140763 we can already repack structs. I have not added support of LLVM `byte` type because it is only available in LLVM22, and we support min-LLVM version 20 afaik. I prefer this approach over the `IITDesc` approach highlighted in rust-lang#140763 because this approach allows code like ```rust #[link_name = "llvm.sqrt.v8bf16"] fn foo(a: u16x8) -> u16x8; ``` which pairs up with the autocasts of rust-lang#140763 to give a nice way to call overloaded intrinsics on `bf16`. Also this approach is a lot less work and more resilient to LLVM changes than the `IITDesc` approach. One important change - the parsing doesn't account for LLVM typed pointers, which were deprecated in LLVM15 and removed in LLVM17, so I didn't bother putting support for them. So, I also removed all uses of typed pointers from the tree. r? @dianqk as you might have more of a context on this due to reviewing the last 2 PRs cc @nikic
|
@bors try jobs=aarch64-gnu |
This comment has been minimized.
This comment has been minimized.
Codegen Overloaded LLVM intrinsics based on their name try-job: aarch64-gnu
|
@bors r- |
|
This pull request was unapproved. This PR was contained in a rollup (#161874), which was unapproved. |
|
💔 Test for 052f00a failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #161398) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
This is a continuation of #140763 - now codegenning overloaded LLVM intrinsics based on their name too. This PR parses the
link_nameof the LLVM intrinsics for the type parameters, partially invertinggetMangledTypeStrandgetIntrinsicNameImplfrom LLVM.There is the concern that @nikic's work on LLVM intrinsics might remove the name mangling, but we can just retain that from the Rust side. I mean even though the LLVM IR wouldn't have the mangling, but we can require that the Rust
link_nameargument contain the mangling. This shouldn't break anything, as existing code already has the name mangling.There is also the concern that this cannot parse
TargetExttypes and non-literal struct types, as their mangling contains their name. If needed in future, we can maybe hardcode some knownTargetExttypes, but currently we don't support it. It also kinda helps that Rust currently cannot handleTargetExttypes. The named struct one is not that big of a problem because courtesy of #140763 we can already repack structs.I have not added support of LLVM
bytetype because it is only available in LLVM22, and we support min-LLVM version 20 afaik.I prefer this approach over the
IITDescapproach highlighted in #140763 because this approach allows code likewhich pairs up with the autocasts of #140763 to give a nice way to call overloaded intrinsics on
bf16. Also this approach is a lot less work and more resilient to LLVM changes than theIITDescapproach.One important change - the parsing doesn't account for LLVM typed pointers, which were deprecated in LLVM15 and removed in LLVM17, so I didn't bother putting support for them. So, I also removed all uses of typed pointers from the tree.
r? @dianqk as you might have more of a context on this due to reviewing the last 2 PRs
cc @nikic