Carry the `b_offset` inside `BackendRepr::ScalarPair` by scottmcm · Pull Request #158666 · rust-lang/rust

scottmcm · 2026-07-01T16:28:12Z

Inspired by rust-lang/compiler-team#1007 but doesn't actually change any of the layout rules just yet.

This turned out to be a nice change even if we didn't use the extra flexibility, IMHO, because it allowed so many things like

@@ -222,12 +224,12 @@ fn from_const_alloc<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
                 let val = read_scalar(offset, size, s, bx.immediate_backend_type(layout));
                 OperandRef { val: OperandValue::Immediate(val), layout, move_annotation: None }
             }
-            BackendRepr::ScalarPair(
-                a @ abi::Scalar::Initialized { .. },
-                b @ abi::Scalar::Initialized { .. },
-            ) => {
+            BackendRepr::ScalarPair {
+                a: a @ abi::Scalar::Initialized { .. },
+                b: b @ abi::Scalar::Initialized { .. },
+                b_offset,
+            } => {
                 let (a_size, b_size) = (a.size(bx), b.size(bx));
-                let b_offset = (offset + a_size).align_to(b.default_align(bx).abi);
                 assert!(b_offset.bytes() > 0);
                 let a_val = read_scalar(
                     offset,

as oh my was that little magic incantation copy-pasted all over the place.

Apologies for the pretty-giant PR. I tried to make it as direct a change as I could: if it was (..) before it's { .. } now, if it was (_, _) before it's { a: _, b: _, b_offset: _ } now. I kept the names the same so the code lines were unchanged even if normally I might have just renamed things, etc. I'll add some inline notes for places of particular interest.

r? @workingjubilee

rustbot · 2026-07-01T16:28:15Z

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a, @makai410

Some changes occurred in compiler/rustc_codegen_llvm/src/builder/autodiff.rs

cc @ZuseZ4

rustbot · 2026-07-01T16:28:18Z

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

scottmcm · 2026-07-01T16:29:48Z

+            ) => {
+                l1.primitive() == r1.primitive()
+                    && l2.primitive() == r2.primitive()
+                    && l_offset == r_offset


note: since this is an eq-like method, I also check that the offsets are the same here. (They always will be right now, but it makes sense to check it regardless IMHO.)

View changes since the review

scottmcm · 2026-07-01T16:45:06Z

+                abi::BackendRepr::ScalarPair { a: out_a, b: out_b, b_offset: out_offset },
+            ) if in_a.size(cx) == out_a.size(cx)
+                && in_b.size(cx) == out_b.size(cx)
+                && in_offset == out_offset =>


note: when transmuting we only want to transmute the immediates individually when the second one is at the same offset in the source and destination.

(It'll probably be UB if they're not, but not necessarily since the out_b might allow uninit, so it's not worth trying to do something smart for those cases. They can fall through to the via-alloca implementation.)

View changes since the review

oh cool, so the code is now more correct also! neat.

I think it was correct before as well because same scalars implied same offset, but it's clearer that it's correct now, and definitely more resiliently correct to subtle changes.

scottmcm · 2026-07-01T16:45:54Z

-            }
+            (
+                Immediate::ScalarPair(a_val, b_val),
+                BackendRepr::ScalarPair { a: _, b: _, b_offset },


Note: by not needing to recompute the offset the two scalars actually ended up unused here.

View changes since the review

scottmcm · 2026-07-01T16:48:20Z

                                a1.size(&self.ecx) == a2.size(&self.ecx)
                                    && b1.size(&self.ecx) == b2.size(&self.ecx)
-                                    // The alignment of the second component determines its offset, so that also needs to match.
-                                    && b1.default_align(&self.ecx) == b2.default_align(&self.ecx)


Note: like the other spot this is thinking about transmutes, so now we can check the offset directly instead of indirectly via the default alignment.

View changes since the review

neat side effect!

scottmcm · 2026-07-01T16:53:53Z

                    "`ScalarPair` second field at bad offset in {inner:#?}",
                );
+                assert_eq!(
+                    b_offset, field2_offset,


Note: for this PR I kept all the existing checks here, since it's not actually changing any layouts. (That's wanting on the MCP FCP.) Just added this new one that the offset stored in the variant matches the expected field offset.

View changes since the review

rustbot · 2026-07-01T17:14:54Z

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

rustbot · 2026-07-01T17:48:48Z

miri is developed in its own repository. If possible, consider making this change to rust-lang/miri instead.

cc @rust-lang/miri

workingjubilee

added some incantations to the PR since the code is now a bit short on them. :^)

a couple minor wording nitpicks, because you wrote r? workingjubilee and I'd hate to disappoint. I will give it another scan once you're done fighting CI and r+ then, but it seems good! my eyes have indeed confirmed "this sure is a lot of whitespace changes thanks to rustfmt".

oh, the callconv code does need a further scan, I'll get to that about then but I don't expect it to need any changes.

View changes since this review

workingjubilee · 2026-07-01T17:56:26Z

-                Immediate::from(if offset.bytes() == 0 {
-                    a_val
-                } else {
-                    assert_eq!(offset, a.size(cx).align_to(b.default_align(cx).abi));


by the shades of the Seraphim!

workingjubilee · 2026-07-01T17:56:46Z

+                let BackendRepr::ScalarPair { a: _, b: _, b_offset } = dest.layout.backend_repr
+                else {
                    bug!("store_with_flags: invalid ScalarPair layout: {:#?}", dest.layout);
                };
-                let b_offset = a_scalar.size(bx).align_to(b_scalar.default_align(bx).abi);


by the hoary hosts of Hoggoth!

workingjubilee · 2026-07-01T17:57:15Z

                        assert_eq!(field.size, a.size(bx.cx()));
                        (Some(a), a_llval)
                    } else {
-                        assert_eq!(offset, a.size(bx.cx()).align_to(b.default_align(bx.cx()).abi));


by the crimson bands of Cyttorak!

workingjubilee · 2026-07-01T18:00:50Z

                                a1.size(&self.ecx) == a2.size(&self.ecx)
                                    && b1.size(&self.ecx) == b2.size(&self.ecx)
-                                    // The alignment of the second component determines its offset, so that also needs to match.
-                                    && b1.default_align(&self.ecx) == b2.default_align(&self.ecx)


neat side effect!

workingjubilee · 2026-07-01T18:03:23Z

            BackendRepr::Scalar(scalar) => PassMode::Direct(scalar_attrs(scalar, Size::ZERO)),
-            BackendRepr::ScalarPair(a, b) => PassMode::Pair(
-                scalar_attrs(a, Size::ZERO),
-                scalar_attrs(b, a.size(cx).align_to(b.default_align(cx).abi)),
-            ),
+            BackendRepr::ScalarPair { a, b, b_offset } => {
+                PassMode::Pair(scalar_attrs(a, Size::ZERO), scalar_attrs(b, b_offset))
+            }


chintap ...I need to look more closely at the surrounding code here because this is actually something that makes me wonder how correct it is.

Or was, for that matter.

Rechecked the maze of code this is entangled with. Yes, this version is correct, and will remain correct.

workingjubilee · 2026-07-01T18:07:55Z

                // We have padding if the sizes don't add up to the total.
+                // (No need to check the offset because it's only possible for the scalar sizes
+                // to add up to the total size if the b_offset equals the `left_size`, but that's
+                // not a sufficient check because there needs to be no padding after right either.)


This could be stated more concisely like so, I think?

Suggested change

// We have padding if the sizes don't add up to the total.

// (No need to check the offset because it's only possible for the scalar sizes

// to add up to the total size if the b_offset equals the `left_size`, but that's

// not a sufficient check because there needs to be no padding after right either.)

// We have padding if the sizes don't add up to the total

// without respect to b_offset, which indicates padding only if the total is higher.

workingjubilee · 2026-07-01T18:10:33Z

+                abi::BackendRepr::ScalarPair { a: out_a, b: out_b, b_offset: out_offset },
+            ) if in_a.size(cx) == out_a.size(cx)
+                && in_b.size(cx) == out_b.size(cx)
+                && in_offset == out_offset =>


oh cool, so the code is now more correct also! neat.

fbstj · 2026-07-01T18:15:18Z

-    /// The two scalars are listed in *memory* order, so the first is at offset zero
-    /// and the second at a non-zero offset.
+    /// The two scalars are listed in *memory* order, so `a` is at offset zero
+    /// and `b` is at non-size offset `b_offset`.


what does non-size offset mean? especially since b_offset: Size

View changes since the review

yeah I left the same review comment. :^) #158666 (comment) resolving it so there's only one instance of this conversation.

I think this happened because of psychically tripping up over "non-zero offset" and "btw b_offset doesn't have to just be a's Size-rounded-up-to-some-Align-again".

oli-obk · 2026-07-01T18:26:38Z

@bors try @rust-timer queue

Carry the `b_offset` inside `BackendRepr::ScalarPair`

rust-bors · 2026-07-01T20:37:03Z

☀️ Try build successful (CI)
Build commit: e1ccfbf (e1ccfbf5eacfbdc172c8ed08bfba6e211f662bd7)
Base parent: e2b71ad (e2b71ade2db4ea263ab0d561d889f3e3795a500d)

rust-timer · 2026-07-01T21:17:37Z

Finished benchmarking commit (e1ccfbf): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -6.9%, secondary -4.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.9%	[-6.9%, -6.9%]	1
Improvements ✅ (secondary)	-4.3%	[-8.8%, -2.1%]	4
All ❌✅ (primary)	-6.9%	[-6.9%, -6.9%]	1

Cycles

Results (primary -2.3%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.3%	[-2.8%, -2.0%]	3
Improvements ✅ (secondary)	-3.6%	[-4.0%, -3.2%]	2
All ❌✅ (primary)	-2.3%	[-2.8%, -2.0%]	3

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.049s -> 485.092s (-1.01%)
Artifact size: 393.28 MiB -> 393.26 MiB (-0.01%)

scottmcm · 2026-07-01T21:22:12Z

Well I was not expecting perf green, especially not on max-rss since this definitely makes BackendRepr bigger, but I'll take it 🤷

workingjubilee · 2026-07-01T21:37:23Z

It is possible that ceasing to recompute the same values over and over might actually remove juuust enough stack usage, in juuust enough places, that it actually matters (esp. not spilling as often), whereas BERepr being bigger gets "handled" by "it's an argument, it floats around in registers".

RalfJung · 2026-07-01T21:40:39Z

max-rss is complete noise in my experience.

rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. F-autodiff `#![feature(autodiff)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2026

rustbot assigned workingjubilee Jul 1, 2026

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 1, 2026

scottmcm commented Jul 1, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

scottmcm commented Jul 1, 2026

View reviewed changes

scottmcm force-pushed the scalar-but-packed branch from 455c80c to ede96e9 Compare July 1, 2026 17:14

rustbot added the T-clippy Relevant to the Clippy team. label Jul 1, 2026

This comment has been minimized.

Sign in to view

workingjubilee approved these changes Jul 1, 2026

View reviewed changes

fbstj reviewed Jul 1, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 1, 2026

rust-bors Bot pushed a commit that referenced this pull request Jul 1, 2026

Auto merge of #158666 - scottmcm:scalar-but-packed, r=<try>

e1ccfbf

Carry the `b_offset` inside `BackendRepr::ScalarPair`

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 1, 2026

scottmcm added 3 commits July 1, 2026 14:20

Carry the b_offset inside BackendRepr::ScalarPair

3440c69

Oh yeah, clippy too

f3e3041

Oh, miri for a different target

1b85fc3

scottmcm force-pushed the scalar-but-packed branch from 22e7592 to 1b85fc3 Compare July 1, 2026 21:22

scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2026

Uh oh!

Uh oh!

Conversation

scottmcm commented Jul 1, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rustbot commented Jul 1, 2026

rustbot commented Jul 1, 2026

scottmcm Jul 1, 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

scottmcm Jul 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm Jul 1, 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

Choose a reason for hiding this comment

scottmcm Jul 1, 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

Choose a reason for hiding this comment

This comment has been minimized.

scottmcm Jul 1, 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

rustbot commented Jul 1, 2026

This comment has been minimized.

rustbot commented Jul 1, 2026

workingjubilee left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbstj Jul 1, 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

workingjubilee Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

This comment has been minimized.

oli-obk commented Jul 1, 2026

This comment has been minimized.

This comment has been minimized.

rust-bors Bot commented Jul 1, 2026

This comment has been minimized.

rust-timer commented Jul 1, 2026

Overall result: no relevant changes - no action needed

scottmcm commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

workingjubilee commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

RalfJung commented Jul 1, 2026

Labels

8 participants

scottmcm commented Jul 1, 2026 •

edited by rustbot

Loading

scottmcm Jul 1, 2026 •

edited by rustbot

Loading

scottmcm Jul 1, 2026 •

edited by rustbot

Loading

scottmcm Jul 1, 2026 •

edited by rustbot

Loading

scottmcm Jul 1, 2026 •

edited by rustbot

Loading

scottmcm Jul 1, 2026 •

edited by rustbot

Loading

workingjubilee left a comment •

edited

Loading

fbstj Jul 1, 2026 •

edited by rustbot

Loading

workingjubilee Jul 1, 2026 •

edited

Loading

scottmcm commented Jul 1, 2026 •

edited

Loading

workingjubilee commented Jul 1, 2026 •

edited

Loading