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?
There was a problem hiding this comment.
David is only back at the end of the week, do you want to wait for that?
There was a problem hiding this comment.
I'm not in a rush with this PR, seems fine to wait.
I have a whole pile of PRs waiting for David it seems. ;)
| 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.
There was a problem hiding this comment.
Doing this post-mono massively degrades the usability of this feature. This code has been reviewed by several members of the types team over several PRs.
There was a problem hiding this comment.
I agree this should be done pre-mono. But this is in the HIR, it should use HIR types, not Layout.
Anyway this is more a little unpleasant surprise I found while writing this PR. Not really something I expect to be resolved in this PR. It seems to work but IMO it should be cleaned up before it turns into technical debt.
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
| // 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.
Doing this post-mono massively degrades the usability of this feature. This code has been reviewed by several members of the types team over several PRs.
| /// Will not peel anything if `self` is a 1-ZST! Callers need to either check | ||
| /// that the result is not a 1-ZST, or have separate logic for that. |
There was a problem hiding this comment.
we could debug_assert! to check?
There was a problem hiding this comment.
Not really, if you look at the callsites I have patched, some will call this on inputs that may be 1-ZST -- but they then check that the output is not a 1-ZST and that's why their use is correct.
| // 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.
David is only back at the end of the week, do you want to wait for that?
5 participants
r? @folkertdev
Cc @beetrees