-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Make Rust ABI return types up to two pointers in registers #124373
Make Rust ABI return types up to two pointers in registers #124373
Conversation
rustbot has assigned @petrochenkov. Use |
This comment has been minimized.
This comment has been minimized.
Oops, that’s kinda weird. I’ll try to reproduce it locally. |
This version is incomplete, but should probably “work” I 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.
There needs to be codegen tests that demonstrate what this actually does.
Are there no tests for such optimizations? |
@@ -786,7 +786,7 @@ fn fn_abi_adjust_for_abi<'tcx>( | |||
// an LLVM aggregate type for this leads to bad optimizations, | |||
// so we pick an appropriately sized integer type instead. | |||
arg.cast_to(Reg { kind: RegKind::Integer, size }); | |||
} else if size <= data_pointer_size * 2 && size.bytes() % 2 == 0 { | |||
} else if size == data_pointer_size * 2 && size.bytes() % 2 == 0 { |
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.
There's no comment or anything explaining this subtlety -- please add it.
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.
Yeah, my original idea on how this should look got shot down by CI, so I'll look into this some more and add comments to the final version.
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.
Right, so it would be useful to understand why the original assumption you made is not compatible with codegen and explain it here. It also may suggest a deeper bug in your implementation, though it's not certain unless you look into it.
The job Click to see the possible cause of the failure (guessed by this bot)
|
Okay, I’ve got it now. Doing it this way produces a load of |
Interesting, I’ll need to verify that these cases are handled. |
Unfortunately, I wasn’t able to circumvent #85265. I’ll tinker with ABI some more and open a new PR if I find something useful. |
Currently functions like
or
pass their return values via stack on
"Rust"
x86_64 ABI (but notably not on"C"
ABI).This PR allows aggregates up to two pointer sizes to be passed in registers, so these functions would be treated as if they returned
(u64, u64)
(treating the second one as if it returnedu128
would be cleaner, but I haven’t yet figured out how to do this and it doesn’t seem to make a difference).This is a perf-motivated change, so I’d really like to see rustc-perf on this change. I’ll try to also do some benchmarks locally.