-
Notifications
You must be signed in to change notification settings - Fork 72
Support scalar pair ABI #381
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
base: main
Are you sure you want to change the base?
Conversation
(T, U) pairs in entrypoints and regular functions are now supported.
|
||
let elem0_ty = self.scalar_pair_element_backend_type(layout, 0, false); | ||
let elem1_ty = self.scalar_pair_element_backend_type(layout, 1, false); | ||
|
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.
@eddyb I am not sure this section is right, can you review?
I'll need this PR for #380, since removing our custom |
That is fortuitous 🤯 |
let elem1_ty = self.scalar_pair_element_backend_type(layout, 1, false); | ||
|
||
let base_ptr = value_ptr.unwrap(); | ||
let ptr1 = bx.inbounds_ptradd(base_ptr, self.const_usize(b_offset.bytes())); |
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.
Why use inbounds_ptradd
here, when it just forwards to inbounds_gep
?
fn inbounds_ptradd(&mut self, ptr: Self::Value, offset: Self::Value) -> Self::Value {
self.inbounds_gep(self.cx().type_i8(), ptr, &[offset])
}
I'm also not so sure you want the resulting type to implicitly be an i8
?
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, this is what I am not sure about, doing pointer arithmetic is bad and I was going to switch it to gep but wanted eddyb to chime in.
Tuple layout is not guaranteed in Rust though. So at least for entrypoints I'd prefer for it to not be supported to avoid UB, forcing user to create an explicit In fact, any type that is not FFI-safe should generate at least compiler warning if not error, just like when you use |
Yeah, I know the repr is not guaranteed in Rust but as you can see in the example we are pushing each piece and then aggregating as a pair on the device side. I guess that is UB though? |
That is certainly not how I'd write in an actual host-side code, I'd simply cast If manually pushing bytes in correct order and adding an extra guarantee about tuple order on shader entrypoint then it might not be UB, but it feels unnatural in Rust. |
Agreed, probably best to be safe and restrict them from entrypoints. |
(T, U) pairs in entrypoints and regular functions are now supported.