peel_transparent_wrappers only works on non-1ZST - #162000
Conversation
|
HIR ty lowering was modified cc @fmease |
rustbot
added
S-waiting-on-review
| // Scalable vector are never 1-ZST. FIXME: is that correct? | ||
| if layout.peel_transparent_wrappers_from_non_1zst(bx).deref().is_scalable_vector() { |
There was a problem hiding this comment.
@davidtwco is it correct that scalable vectors are never 1ZST?
What does is_sized even say for them?
| if layout.peel_transparent_wrappers_from_non_1zst(self).ty.is_scalable_vector() { | ||
| let vscale = self.vscale(self.type_i64()); | ||
| self.mul(vscale, bytes) | ||
| } else { | ||
| bytes |
There was a problem hiding this comment.
This code seems quite fragile, why does it need to special-case scalable vectors and how do we know there aren't any other special cases that have been forgotten here?
But that's pre-existing... still, quite concerning IMO. Cc @davidtwco @ZuseZ4
| // Accept (transparently wrapped) scalar 64-bit primitives. | ||
| matches!( | ||
| layout.peel_transparent_wrappers(&cx).ty.kind(), | ||
| layout.peel_transparent_wrappers_from_non_1zst(&cx).ty.kind(), |
There was a problem hiding this comment.
HIR uses layout?!? That's a pretty stark layering violation, isn't it?
There was a problem hiding this comment.
With CMSE we can ensure that this works out. We need to know how many registers will be used by the signature to error (even in cargo check builds) when the arguments don't fit in the available space.
There was a problem hiding this comment.
I didn't doubt that it works, but it's a hack in terms of compiler layering. It's one more step towards an unmaintainable spaghetti mess.
This comment has been minimized.
This comment has been minimized.
f539126 to
973ef1d
Compare
| // So we never treat 1-ZST indirectly. | ||
| if self.is_1zst() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This basically says that the #[rustc_pass_indirectly_in_non_rustic_abis] attribute is a NOP on 1-ZST. I think that's reasonable. A better approach would be to have a non_trivial_abi_field and declare the attribute to make the ABI non-trivial, but before #157973 lands I don't think it makes sense to implement anything like that and anyway the attribute is perma-unstable.
But I am not sure where to document it, the attribute seems wholly undocumented?
There was a problem hiding this comment.
I think leaving a FIXME for making this non_trivial_abi_field after #157973 is merged (since it's already in proposed FCP) is fine for now. I think #[rustc_pass_indirectly_in_non_rustic_abis] is primarily documented on TyAndLayout::pass_indirectly_in_non_rustic_abis and in the VaList implementation at the moment.
There was a problem hiding this comment.
At the moment it's just an implementation detail of c-variadic functions.
There was a problem hiding this comment.
At the moment it's just an implementation detail of c-variadic functions.
I know that's the intent, but even then it needs docs that say that. ;)
But we're generally not doing great regarding documentation of rustc attributes.
I can add a FIXME.
973ef1d to
e7b7d5b
Compare
5 participants
r? @folkertdev
Cc @beetrees