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
Merged
merged 1 commit into from
Jul 1, 2025
Merged

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

merged 1 commit into from
Jul 1, 2025

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
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@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
Disable f16 on Aarch64 without neon for llvm < 20.1.1
4fd37c7 
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 f717f44 to 4fd37c7 Compare June 30, 2025 14:47
@cuviper
Copy link
Member

cuviper commented Jun 30, 2025

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 30, 2025

📌 Commit 4fd37c7 has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 1, 2025
Merged
bors added a commit that referenced this pull request Jul 1, 2025
@bors
Auto merge of #143267 - matthiaskrgr:rollup-suvzar6, r=matthiaskrgr
f46ce66 
Rollup of 8 pull requests

Successful merges:

 - #143125 (Disable f16 on Aarch64 without neon for llvm < 20.1.1)
 - #143156 (inherit `#[align]` from trait method prototypes)
 - #143178 (rustdoc default faviocon)
 - #143234 (Replace `ItemCtxt::report_placeholder_type_error` match with a call to `TyCtxt::def_descr`)
 - #143245 (mbe: Add tests and restructure metavariable expressions)
 - #143257 (Upgrade dependencies in run-make-support)
 - #143263 (linkify CodeSuggestion in doc comments)
 - #143264 (fix: Emit suggestion filename if primary diagnostic span is dummy)

Failed merges:

 - #143251 (bootstrap: add build.tidy-extra-checks option)

r? `@ghost`
`@rustbot` modify labels: rollup
@tgross35
Copy link
Contributor Author

tgross35 commented Jul 1, 2025

Not that I think you'll necessarily want to backport this for Fedora, but the compiler-builtins change is at rust-lang/compiler-builtins#963.

@bors bors merged commit 74d1d59 into rust-lang:master Jul 1, 2025
10 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 1, 2025
@tgross35 tgross35 deleted the aarch64-neon-llvm19-f16 branch July 1, 2025 08:33
rust-timer added a commit that referenced this pull request Jul 1, 2025
@rust-timer
Unrolled build for #143125
a7c7f22 
Rollup merge of #143125 - tgross35:aarch64-neon-llvm19-f16, r=cuviper

Disable f16 on Aarch64 without neon for llvm < 20.1.1

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
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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.90.0
Development

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