Add assembly test for -Zreg-struct-return option
#145382
Conversation
rustbot
added
S-waiting-on-review
|
Thanks for this!
I would suggest preserving these two lines in the tests themselves.
Yeah. It would also be a good idea to test the other side of that threshold, i.e. testing a size where it does not happen even with the flag enabled. (Also, a smaller size that puts both fields in the same register like in your CE example wouldn't hurt either, but I am not sure what the threshold for "too many tests" is in Rust) |
Added tests for both of those cases, we can still decide to remove them if they turn out to be too much |
|
The tests look great now with the comments and the extra cases. |
|
I now also adapted the 8 bytes tests to be less rigid on the instruction stream, thank you for all the feedback! |
|
Hey @tgross35, thank you for the feedback. I separated the tests into the caller/callee sides, and I added the CHECK-DAG directives for instruction streams where ordering may vary in the future. |
rustbot
added
S-waiting-on-author
winstonallo
force-pushed
the
reg-struct-return-asm-test
branch
from
b7c5825 to
0860ac5
Compare
Done :) |
…n` for structs of different sizes. This test covers: * The callee side, making sure that the structs are correctly loaded into registers when `-Zreg-struct-return` is enabled * The caller side, making sure that callers do receive returned structs in registers when `-Zreg-struct-return` is enabled Structs of the size of up to 2 registers (8 bytes) can be returned in registers in x86_32. Therefore, the tests are done with 3 different struct sizes: * 2 bytes (register returns should happen) * 8 bytes (last value where register returns should happen) * 12 bytes (register returns should not happen even when `-Zreg-struct-return` is enabled)
winstonallo
force-pushed
the
reg-struct-return-asm-test
branch
from
0860ac5 to
5f39612
Compare
|
Thank you! @bors r+ |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - #145382 (Add assembly test for `-Zreg-struct-return` option) - #145746 (Fix STD build failing for target_os = "espidf") - #145826 (Use AcceptContext in AttribueParser::check_target) - #145894 (Ensure the coordinator thread terminates before its channels drop) - #145946 (Remove unnecessary `[dependencies.unicode-properties]` entries.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #145382 - winstonallo:reg-struct-return-asm-test, r=tgross35 Add assembly test for `-Zreg-struct-return` option r? `@tgross35` As discussed in #145309 with `@tgross35` and `@ojeda,` I added assembly tests for the `-Zreg-struct-return` option verifying that it changes the ABI from hidden pointer to register-return on x86_32. The test covers: - Direct struct construction, showing register return vs hidden pointer - External function calls returning structs, showing ABI mismatch handling Different memory layouts affect ABI mismatch handling, but register returns use the same register allocation regardless of struct field layout (apart from the fact that they use smaller registers for smaller structs, of course). [Here](https://godbolt.org/z/dcW6rnMG3) is a compiler explorer with 2 examples. Let me know if there is anything more I could add. Since register returns only happen for structs up to the size of 2 registers, I figured testing the pivot value (8 bytes) would be most critical.
r? @tgross35
As discussed in #145309 with @tgross35 and @ojeda, I added assembly tests for the
-Zreg-struct-returnoption verifying that it changes the ABI from hidden pointer to register-return on x86_32.The test covers:
Different memory layouts affect ABI mismatch handling, but register returns use the same register allocation regardless of struct field layout (apart from the fact that they use smaller registers for smaller structs, of course).
Here is a compiler explorer with 2 examples. Let me know if there is anything more I could add. Since register returns only happen for structs up to the size of 2 registers, I figured testing the pivot value (8 bytes) would be most critical.