-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Ensure floats are returned losslessly by the Rust ABI on 32-bit x86 #123351
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
Conversation
|
rustbot has assigned @compiler-errors. Use |
compiler-errors
left a comment
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.
Changes lgtm, but I'm not an ABI expert
|
so let's get another look at it r? compiler |
|
Not an ABI expert either r? compiler |
This comment has been minimized.
This comment has been minimized.
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.
Looks reasonable to me, too. I'm also not an expert on this matter ^^'.
@RalfJung, could you help us out?
c413b2f to
c42c4b5
Compare
|
I've added a commit to update |
My name appears in these issues as I gained just enough knowledge to identify that we have a problem, but I don't feel confident to approve any changes to our ABI, sorry. With my limited knowledge however, this change does make a lot of sense. And it's a lot simpler than what I thought was necessary to do this. Very nice! So... who is an ABI expert? @bjorn3, @nikic and @eddyb are the first people coming to my mind. (Maybe we should have some sort of dedicated ABI expert ping group. It would be really good to have some people in charge of ABI-related questions.) |
c42c4b5 to
852ffd3
Compare
|
I guess it can be left for a followup, but if SSE2 is available, I think |
|
@programmerjake See #115919 - currently blocked on #116584. I agree it would be a good future improvement once the target feature issues have been sorted out. |
|
I don't have time to review this for the next two weeks at least unfortunately. |
|
No worries. I'll try someone else. r? @nikic |
|
💔 Test failed - checks-actions |
Error appears to be spurious. |
|
@bors r=nikic,workingjubilee |
|
💡 This pull request was already approved, no need to approve it again.
|
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (c6727fc): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
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: 703.434s -> 705.609s (0.31%) |
Solves #115567 for the (default)
"Rust"ABI. When compiling for 32-bit x86, this PR changes the"Rust"ABI to return floats indirectly instead of in x87 registers (with the exception of singlef32s, which this PR returns in general purpose registers as they are small enough to fit in one). No change is made to the"C"ABI as that ABI requires x87 register usage and therefore will need a different solution.