-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Update the minimum external LLVM to 19 #139275
Conversation
Some changes occurred in coverage tests. cc @Zalathar This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. |
This comment has been minimized.
This comment has been minimized.
@@ -270,20 +269,9 @@ pub(crate) fn to_llvm_features<'a>(sess: &Session, s: &'a str) -> Option<LLVMFea | |||
("aarch64", "fhm") => Some(LLVMFeature::new("fp16fml")), | |||
("aarch64", "fp16") => Some(LLVMFeature::new("fullfp16")), | |||
// Filter out features that are not supported by the current LLVM version | |||
("aarch64", "fpmr") if get_version().0 != 18 => None, | |||
("aarch64", "fpmr") => None, // only existed in 18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to retain this feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure in which context it would have been specified and needed to be filtered out before. e.g. was that only from unstable #[target_feature]
or -Ctarget-feature
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrkajetanp Do we need to keep this target feature around?
It looks like there is some runtime detection support for it: https://github.com/rust-lang/stdarch/blob/65712f37fb4d8f275e5eb7bd7b0057fdd68a91aa/crates/std_detect/src/detect/arch/aarch64.rs#L162-L164
I'm not super clear on how/why we're supporting target features that LLVM doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relevant discussion was here: #128192
The approach we settled on was this:
We switch it on by default through target specs on LLVM 18, it's already switched on on LLVM 19+ and it doesn't exist on older LLVM versions. But we do keep the detection in std_detect.
So, if we no longer support LLVM 18 then it should not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to remove it from stdarch
first though, right? If so, then I suggest:
- Merge this PR with the
=> None
filter. - Remove "fpmr" from
stdarch
. - Update
stdarch
and drop that line in a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it would be nice to add an appropriate FIXME comment to this PR that mentions the stdarch
plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be desirable to keep it in stdarch for runtime detection. You no longer need to enable it for LLVM to use it, but you may still want to check whether the machine your code runs on actually supports it.
Okay, I think we can figure out the details of the fpmr handling as a followup... @bors r+ |
Update the minimum external LLVM to 19 With this change, we'll have stable support for LLVM 19 and 20. For reference, the previous increase to LLVM 18 was rust-lang#130487. cc `@rust-lang/wg-llvm` r? nikic
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#138546 (Add integer to string formatting tests) - rust-lang#138950 (replace extra_filename with strict version hash in metrics file names) - rust-lang#139028 (Make target maintainers more easily pingable) - rust-lang#139274 (Rustdoc: typecheck settings.js) - rust-lang#139275 (Update the minimum external LLVM to 19) - rust-lang#139328 (Fix 2024 edition doctest panic output) Failed merges: - rust-lang#138947 (Refactor Apple version handling in the compiler) r? `@ghost` `@rustbot` modify labels: rollup
@bors r- rollup=iffy |
That just needed a line-number @bors r=nikic |
Update the minimum external LLVM to 19 With this change, we'll have stable support for LLVM 19 and 20. For reference, the previous increase to LLVM 18 was rust-lang#130487. cc `@rust-lang/wg-llvm` r? nikic
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
@bors r=nikic |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 5e17a2a (parent) -> c211076 (this PR) Test differencesShow 56 test diffsStage 1
Stage 2
Job group index
Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (c211076): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 776.183s -> 775.586s (-0.08%) |
With this change, we'll have stable support for LLVM 19 and 20.
For reference, the previous increase to LLVM 18 was #130487.
cc @rust-lang/wg-llvm
r? nikic