peel_transparent_wrappers only works on non-1ZST by RalfJung · Pull Request #162000 · rust-lang/rust

RalfJung · 2026-08-30T07:33:39Z

rustbot · 2026-08-30T07:33:42Z

HIR ty lowering was modified

RalfJung · 2026-08-30T07:35:05Z

+        // Scalable vector are never 1-ZST. FIXME: is that correct?
+        if layout.peel_transparent_wrappers_from_non_1zst(bx).deref().is_scalable_vector() {


@davidtwco is it correct that scalable vectors are never 1ZST?
What does is_sized even say for them?

View changes since the review

David is only back at the end of the week, do you want to wait for that?

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. ;)

I believe it's correct that a scalable vector will never be a 1ZST.

Do you believe it hard enough to stake soundness of the feature on it? ;)

RalfJung · 2026-08-30T07:36:44Z

+                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


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

View changes since the review

This needs to special-case scalable vectors because this code is used when we generate a memcpy. The size being memcpyd for a scalable vector is the element_size * vunit * vscale and as vscale is a runtime constant that we need to insert a call to get - that's what this does. Otherwise we memcpy only parts of vectors and get incorrect results.

It is a bit fragile. It's hard to address properly without making layout.size an enum that could be scalable or fixed length and accounting for this case everywhere - I experimented with this locally and it's a massive change. I'd like to do something like it to be more confident that we're not missing any cases, but I wanted to spend some time experimenting first with how best to make that change so that I didn't waste anyone's time with it. I did check for other places that this might be necessary and couldn't find any, so I don't believe it's pressing immediately.

So in other words, scalable vectors aren't actually sized. Does layout.is_sized() return true for them? That's definitely a bad spot to be in, and something that should be cleaned up before anything here becomes stable.

RalfJung · 2026-08-30T07:37:44Z

    // 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(),


HIR uses layout?!? That's a pretty stark layering violation, isn't it?

View changes since the review

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.

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.

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.

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.

RalfJung · 2026-08-30T08:05:49Z

+        // So we never treat 1-ZST indirectly.
+        if self.is_1zst() {
+            return false;
+        }


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?

View changes since the review

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.

At the moment it's just an implementation detail of c-variadic functions.

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.

folkertdev · 2026-08-31T17:20:00Z

    // 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(),


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.

folkertdev · 2026-08-31T17:21:43Z

+    /// 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.


we could debug_assert! to check?

View changes since the review

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.

folkertdev · 2026-08-31T17:23:38Z

+        // Scalable vector are never 1-ZST. FIXME: is that correct?
+        if layout.peel_transparent_wrappers_from_non_1zst(bx).deref().is_scalable_vector() {


David is only back at the end of the week, do you want to wait for that?

RalfJung · 2026-09-04T14:18:43Z

+                // FIXME(rustc_scalable_vector/stdarch_aarch64_sve): Scalable vectors aren't
+                // actually sized, but we pretend they are. Here we have to hack around that.


@davidtwco is there a standard mnemonic you are using to mark FIXMEs related to this effort, something you will grep for before anything gets stabilized to ensure it was all taken care of? Usually we use the name of the language feature, but strangely there is apparently no language feature for scalable vectors.

View changes since the review

RalfJung · 2026-09-04T14:21:01Z

        PlaceValue::new_sized(
-            bx.alloca_with_ty(layout.peel_transparent_wrappers(bx)),
+            // FIXME why is this peeling at all? And why is it redoing the work the caller just did?
+            bx.alloca_with_ty(layout.peel_transparent_wrappers_from_non_1zst(bx)),


FWIW this is also a scalable vector question. I had a hard time making sense of the code for sve in this file.

View changes since the review

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 30, 2026

rustbot assigned folkertdev Aug 30, 2026

RalfJung commented Aug 30, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the peel_transparent_wrappers branch from f539126 to 973ef1d Compare August 30, 2026 08:05

RalfJung commented Aug 30, 2026

View reviewed changes

peel_transparent_wrappers only works on non-1ZST

e7b7d5b

RalfJung force-pushed the peel_transparent_wrappers branch from 973ef1d to e7b7d5b Compare August 30, 2026 12:33

folkertdev reviewed Aug 31, 2026

View reviewed changes

RalfJung force-pushed the peel_transparent_wrappers branch from 86d5cd3 to 8f0ae1f Compare September 4, 2026 14:16

RalfJung commented Sep 4, 2026

View reviewed changes

leave a comment for some sve hacks

d91973f

RalfJung force-pushed the peel_transparent_wrappers branch from 8f0ae1f to d91973f Compare September 4, 2026 14:22

		// Scalable vector are never 1-ZST. FIXME: is that correct?
		if layout.peel_transparent_wrappers_from_non_1zst(bx).deref().is_scalable_vector() {

		/// 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.

		// FIXME(rustc_scalable_vector/stdarch_aarch64_sve): Scalable vectors aren't
		// actually sized, but we pretend they are. Here we have to hack around that.

Uh oh!

Uh oh!

Conversation

RalfJung commented Aug 30, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 30, 2026

Uh oh!

RalfJung Aug 30, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Aug 30, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Aug 30, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

RalfJung Aug 30, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

folkertdev Aug 31, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Sep 4, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Sep 4, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RalfJung commented Aug 30, 2026 •

edited by rustbot

Loading

RalfJung Aug 30, 2026 •

edited by rustbot

Loading

RalfJung Aug 30, 2026 •

edited by rustbot

Loading

RalfJung Aug 30, 2026 •

edited by rustbot

Loading

RalfJung Aug 30, 2026 •

edited by rustbot

Loading

RalfJung Aug 30, 2026 •

edited

Loading

folkertdev Aug 31, 2026 •

edited by rustbot

Loading

RalfJung Sep 4, 2026 •

edited by rustbot

Loading

RalfJung Sep 4, 2026 •

edited by rustbot

Loading