Skip to content

Disable f16 on Aarch64 without neon for llvm < 20.1.1 #143125

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Disable f16 on Aarch64 without neon for llvm < 20.1.1 #143125

wants to merge 2 commits into from
+8 −0

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jun 28, 2025 •
edited
Loading

This check was added unconditionally in c51b229 ("Disable f16 on Aarch64 without neon") and reverted in 4a8d357 ("Revert "Disable f16 on Aarch64 without neon"") since it did not fail in Rust's build. However, it is still possible to hit this crash if using LLVM 19 built with assertions, so disable the type conditionally based on version here.

Note that for these builds, a similar patch is needed in the build script for compiler-builtins since it does not yet use cfg(target_has_reliable_f16) (hopefully to be resolved in the near future).

Report: #139276 (comment)
Original LLVM issue: llvm/llvm-project#129394

r? @cuviper

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 28, 2025
@tgross35 tgross35 mentioned this pull request Jun 28, 2025
Merged
@tgross35 tgross35 force-pushed the aarch64-neon-llvm19-f16 branch from 4292cd7 to a6d4975 Compare June 28, 2025 01:37
@tgross35
Copy link
Contributor Author

Note that for Fedora, you will also need to add a similar patch here https://github.com/rust-lang/compiler-builtins/blob/674910e0fa6f0fb2cc055f4f7051ff0eb53c7735/compiler-builtins/configure.rs#L91 that checks target.features. This can't really be done in r-l/r since we don't have access to the LLVM version when building builtins. Now that builtins is a subtree I am planning to switch it to use cfg(target_has_eliable_f16), but that of course won't be in time for the version you are building.

cuviper
cuviper reviewed Jun 28, 2025
View reviewed changes
compiler/rustc_codegen_llvm/src/llvm_util.rs Outdated
if !sess.target_features.iter().any(|f| f.as_str() == "neon")
&& version < (20, 1, 1) =>
{
panic!("hi");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is leftover from a local test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃 yup, was double checking the features filter and apparently forgot to save

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tgross35
Disable f16 on Aarch64 without neon for llvm < 20.1.1
950b096 
This check was added unconditionally in c51b229 ("Disable f16 on
Aarch64 without `neon`") and reverted in 4a8d357 ("Revert "Disable
`f16` on Aarch64 without `neon`"") since it did not fail in Rust's
build. However, it is still possible to hit this crash if using LLVM 19
built with assertions, so disable the type conditionally based on
version here.

Note that for these builds, a similar patch is needed in the build
script for `compiler-builtins` since it does not yet use
`cfg(target_has_reliable_f16)` (hopefully to be resolved in the near
future).

Report: https://www.github.com/rust-lang/rust/pull/139276#issuecomment-3014781652
Original LLVM issue: https://www.github.com/llvm/llvm-project/issues/129394
@tgross35 tgross35 force-pushed the aarch64-neon-llvm19-f16 branch from a6d4975 to 950b096 Compare June 28, 2025 01:46
@cuviper
Copy link
Member

cuviper commented Jun 28, 2025

Note that for Fedora, you will also need to add a similar patch here [...]

Ok, makes sense.

Now that builtins is a subtree I am planning to switch it to use cfg(target_has_eliable_f16), but that of course won't be in time for the version you are building.

Right, and so this PR won't be enough on its own for master either, but it may still make sense to land it in anticipation of that subtree change.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tgross35
Copy link
Contributor Author

Oh, that's interesting; Cranelift now supports these types even though LLVM 19 doesn't. The situation here looks kind of weird; IIUC cranelift is building coretests but must be running it against LLVM-built core?

@cuviper
Copy link
Member

cuviper commented Jun 29, 2025

I'm not sure, but aarch64-unknown-linux-gnu should have neon, right?
So this PR should still think it has_reliable_f16?

cuviper
cuviper reviewed Jun 29, 2025
View reviewed changes
@tgross35 @cuviper
fixup! 950b096
f717f44 
Co-authored-by: Josh Stone <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper left review comments

Assignees

@cuviper cuviper

Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tgross35 @rust-log-analyzer @cuviper @rustbot