more complete sparc64 ABI fix for aggregates with floating point members #94216
Conversation
Previous fix didn't handle nested structures at all.
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
This definitely would benefit from some additional test coverage. |
Yes. My main test was to build Firefox. With my previous fix there was some improvement. But later I have found that there was also regression and part of what started to work was rather coincidence. This time newly Firefox Webrender started work and all previous known issues remain resolved. The issue with tests is that they won't cover everything. I have used following tests here: Note that these tests are not platform specific. I mean there could be teoretically also other platforms which could benefit from them. So I'm not entirely sure how to proceed here. What do you exactly suggest? |
|
I would like to see codegen tests at least for the cases that involve the problematic nested structures in question. Either LLVM or assembly works (much like what was added in #91003) It is OK if they aren't 100% exhaustive, as the primary purpose is for them to act as a regression test. A run-pass test based on your test code is also a welcome addition. Especially if running the entire rustc test suite on sparc passed without any failures before this fix. |
|
@nagisa is it ok now with one test case added? |
|
r? @nagisa @bors r+ NB: I didn't dig into the changes very deeply. I don't have a SPARC ABI document on hand and most of the stuff I saw, while complicated, seemed reasonable in isolation. Since this is a T2/3 target and @psumbera has been one of the very few parties interested in maintaining the target, I'm inclined to trust them implicitly that this is an improvement over the status quo. |
|
📌 Commit 992c27c has been approved by |
bors
added
S-waiting-on-bors
more complete sparc64 ABI fix for aggregates with floating point members Previous fix didn't handle nested structures at all.
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (edda7e9): comparison url. Summary: This benchmark run did not return any relevant results. 2 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
| if scalar1.value == abi::F32 { | ||
| offset += Reg::f32().size; | ||
| } else if scalar2.value == abi::F64 { | ||
| offset += Reg::f64().size; | ||
| } else if let abi::Int(i, _signed) = scalar1.value { | ||
| offset += i.size(); | ||
| } else if scalar1.value == abi::Pointer { | ||
| offset = offset + Reg::i64().size; | ||
| } | ||
|
|
||
| // This doesn't intentionally handle structures with floats which needs | ||
| // special care below. | ||
| if let Some(uniform) = is_homogeneous_aggregate(cx, arg) { | ||
| arg.cast_to(uniform); | ||
| return; | ||
| if (offset.raw % 4) != 0 && (scalar2.value == abi::F32 || scalar2.value == abi::F64) { | ||
| offset.raw += 4 - (offset.raw % 4); | ||
| } |
There was a problem hiding this comment.
There's three issues here:
Abi::ScalarPairshould not be special-cased in call ABIs - it's justAbi::Aggregatewith extra Rust-only optimizations tacked on (none of the other issues matter if theparse_structurecodepath is taken)- this is not how one computes the offset of the second scalar for an
Abi::ScalarPair(scalar1.size(cx).align_to(scalar2.align(cx).abi)is more or less the typical formula, anything else risks being wrong for certain scalars) else if scalar2.value == abi::F64is probably a typo? certainly seems like the intention is to get the size ofscalar1(which is justscalar1.size(cx))
As it stands, I believe this code passes (u128, f64) incorrectly (thanks to alignment, I can't think of anything else that would be mismatched), so it was probably unobserved for that reason.
| for i in 0..layout.fields.count().clone() { | ||
| if offset < layout.fields.offset(i) { | ||
| offset = layout.fields.offset(i); | ||
| } |
There was a problem hiding this comment.
This is a silent failure (it will cause data corruption AFAICT). If the fields aren't in ascending order, this is a non-C-friendly type and you have to either ICE (which is arguably unfriendly), or better just fall back to "this cannot be passed in any optimized way".
Sadly SPARC64 seems to be one of the architectures unfortunate enough to use "turn struct into as many immediate arguments as needed", instead of byval, for on-stack passing.
But still, you could use something similar to the Union case handled a bit above.
Actually, wait, this is wronger than I thought, it doesn't add field offsets to the enclosing struct's offset, just replaces it altogether.
There's no way the testcases added in this PR could work, I think it's just hitting all the silent failure modes around here and last_offset and accidentally causing the right output...
There was a problem hiding this comment.
Thanks for looking at it! I'm sure there are problems with my fix.
I was testing it with: https://github.com/psumbera/rust-sparc64-abi-tests
So maybe you can come with sample code which will not work.
Or If you are willing to help with your expertise it I'm ready to provide SPARC testing.
Previous fix didn't handle nested structures at all.