-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 LLVM submodule #107879
Update LLVM submodule #107879
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) soon. Please see the contribution instructions for more information. |
|
Can you add a regression test for this? Probably a ui |
rust/tests/ui/numbers-arithmetic/saturating-float-casts-impl.rs Lines 350 to 380 in eebdfb5
It seems that this part doesn't check unsigned to float casting for edge cases. We need to add unsigned to float casting checks and modify |
I think that's fine to add separately, now that there's a targeted test here. @bors r+ |
📌 Commit 1c97cde6f11dab37312faef4a2460339115ae3c8 has been approved by It is now in the queue for this repository. |
⌛ Testing commit 1c97cde6f11dab37312faef4a2460339115ae3c8 with merge 97abed1fd6352f808f6309387c82f36fcc9959e3... |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
Those values failed equality on |
@bors r+ |
📌 Commit 1e4ba39b026dff0378b3612d12da1a9f2e02f128 has been approved by It is now in the queue for this repository. |
I force-pushed to parent commit by accident. Now ready for review. |
I don't think |
The u64 to f32 conversion is currently implemented using x87 even when SSE2 is enabled. If in the future the compiler changes to use SSE2 for that conversion, it will be difficult to know if there is a problem with the x87 implementation as the test will not fail as long as SSE2 is enabled. |
But I think it's okay for us to just use the SSE2 version for now. As long as SSE2 is enabled, we can generate code that works fine on x86. I will contribute to solving the x87 issue on the LLVM side later. |
The LLVM fix is specific to Windows, and @bors r+ |
Update LLVM submodule Fixes rust-lang#105626.
Update LLVM submodule Fixes rust-lang#105626.
⌛ Testing commit 313f04f with merge 2f5bc1f0b395c73a727be74e7f1b277a36a386c3... |
💔 Test failed - checks-actions |
@rust-lang/bootstrap -- this should be expected! We're updating LLVM, but not all CI builders use that. |
@cuviper oops, this might be the first time LLVM's been updated since #107234 - I think we should skip this test altogether if LLVM was updated in the PR. Maybe take the code in Lines 219 to 230 in ae5473c
|
@jyn514 I gave that a shot, let me know what you think! |
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 approve the last commit (91af58ac58dee5794fe54a8f9cb5556d3585688d).
Just a minor nit, if you want.
@bors r+ |
This comment has been minimized.
This comment has been minimized.
☀️ Test successful - checks-actions |
Finished benchmarking commit (18caf88): 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. |
Fixes #105626