compiler: Document the offset invariant of `OperandValue::Pair` #142044

workingjubilee · 2025-06-04T21:36:05Z

A subtle invariant of OperandValue::Pair that came up during review and was found to be undocumented.

Visible in code like this:

rust/compiler/rustc_codegen_ssa/src/mir/operand.rs

Lines 376 to 392 in 4b27a04

    
           let (in_scalar, imm) = match (self.val, self.layout.backend_repr) { 
        
               // Extract a scalar component from a pair. 
        
               (OperandValue::Pair(a_llval, b_llval), BackendRepr::ScalarPair(a, b)) => { 
        
                   if offset.bytes() == 0 { 
        
                       assert_eq!(field.size, a.size(bx.cx())); 
        
                       (Some(a), a_llval) 
        
                   } else { 
        
                       assert_eq!(offset, a.size(bx.cx()).align_to(b.align(bx.cx()).abi)); 
        
                       assert_eq!(field.size, b.size(bx.cx())); 
        
                       (Some(b), b_llval) 
        
                   } 
        
               } 
        
               _ => { 
        
                   span_bug!(fx.mir.span, "OperandRef::extract_field({:?}): not applicable", self) 
        
               } 
        
           };

r? @scottmcm

rustbot · 2025-06-04T21:36:11Z

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

workingjubilee · 2025-06-04T21:42:25Z

compiler/rustc_codegen_ssa/src/mir/operand.rs

@@ -45,9 +45,11 @@ pub enum OperandValue<V> {
    Immediate(V),
    /// A pair of immediate LLVM values. Used by wide pointers too.
    ///
-    /// An `OperandValue` *must* be this variant for any type for which
+    /// # Invariants
+    /// - for `Pair(a, b)`, `a` is always offset 0 but **may** be `FieldIdx::ONE`


not entirely sure if this is more of a "by definition" thing or if it requires manually enforcing it. not entirely sure if anyone knows.

You might consider elaborating in a later paragraph.

For example, if you discuss struct Foo((), u8, (), u32, ());, today that's a ScalarPair where the Pair(a, b) has a as FieldIdx(3) and b as FieldIdx(1).

(We shouldn't be using LLVM struct types to do this any more, but I think originally it had to be that way so that if you loaded the pair type from memory they'd go into the correct fields. Now it's just that that's the definition of what a ScalarPair layout even means, I think.)

Hm.

Is b guaranteed to not be at offset 0, then?

better, worse, incorrect?

Is b guaranteed to not be at offset 0, then?

Yes. Neither of the immediates can be a unit -- we don't bother making backend immediate values for a nothing-unit. So both scalars have non-zero size (which you can also see by inspection of https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.Scalar.html ) and thus they necessarily have two different offsets.

scottmcm · 2025-06-05T00:01:29Z

Thanks! This looks good to me.

@bors r+ rollup

bors · 2025-06-05T00:01:32Z

📌 Commit 64df9e3 has been approved by scottmcm

It is now in the queue for this repository.

…ue-pair, r=scottmcm compiler: Document the offset invariant of `OperandValue::Pair` A subtle invariant of `OperandValue::Pair` that came up during review and was found to be undocumented. Visible in code like this: https://github.com/rust-lang/rust/blob/4b27a04cc8ed4da10a546a871e23e665d03f7a79/compiler/rustc_codegen_ssa/src/mir/operand.rs#L376-L392

Rollup of 11 pull requests Successful merges: - #125087 (Optimize `Seek::stream_len` impl for `File`) - #141982 (`tests/ui`: A New Order [5/N]) - #142012 (Replace some `Option<Span>` with `Span` and use DUMMY_SP instead of None) - #142044 (compiler: Document the offset invariant of `OperandValue::Pair`) - #142047 (Ensure stack in two places that affect s390x) - #142058 (Clean `rustc_attr_parsing/src/lib.rs` documentation) - #142067 (canon_abi: make to_erased_extern_abi just a detail in formatting) - #142072 (doc: Fix inverted meaning in E0783.md) - #142084 (add myself to rotation) - #142091 (Fix AIX build) - #142092 (rustdoc: Support middle::ty associated const equality predicates again) Failed merges: - #142042 (Make E0621 missing lifetime suggestion verbose) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #142044 - workingjubilee:document-operandvalue-pair, r=scottmcm compiler: Document the offset invariant of `OperandValue::Pair` A subtle invariant of `OperandValue::Pair` that came up during review and was found to be undocumented. Visible in code like this: https://github.com/rust-lang/rust/blob/4b27a04cc8ed4da10a546a871e23e665d03f7a79/compiler/rustc_codegen_ssa/src/mir/operand.rs#L376-L392

rustbot assigned scottmcm Jun 4, 2025

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 Jun 4, 2025

workingjubilee commented Jun 4, 2025

View reviewed changes

compiler: Document the offset invariant of OperandValue::Pair

64df9e3

workingjubilee force-pushed the document-operandvalue-pair branch from fc26e4e to 64df9e3 Compare June 4, 2025 23:43

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2025

GuillaumeGomez mentioned this pull request Jun 5, 2025

Rollup of 12 pull requests #142096

Closed

matthiaskrgr mentioned this pull request Jun 5, 2025

Rollup of 11 pull requests #142099

Merged

workingjubilee mentioned this pull request Jun 6, 2025

Allow enum and union literals to also create SSA values #138759

Draft

bors merged commit 29954af into rust-lang:master Jun 6, 2025
10 checks passed

rustbot added this to the 1.89.0 milestone Jun 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

compiler: Document the offset invariant of `OperandValue::Pair` #142044

compiler: Document the offset invariant of `OperandValue::Pair` #142044

workingjubilee commented Jun 4, 2025

Uh oh!

rustbot commented Jun 4, 2025

Uh oh!

workingjubilee Jun 4, 2025

Uh oh!

scottmcm Jun 4, 2025

Uh oh!

workingjubilee Jun 4, 2025

Uh oh!

workingjubilee Jun 4, 2025

Uh oh!

scottmcm Jun 4, 2025

Uh oh!

scottmcm commented Jun 5, 2025

Uh oh!

bors commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

	let (in_scalar, imm) = match (self.val, self.layout.backend_repr) {
	// Extract a scalar component from a pair.
	(OperandValue::Pair(a_llval, b_llval), BackendRepr::ScalarPair(a, b)) => {
	if offset.bytes() == 0 {
	assert_eq!(field.size, a.size(bx.cx()));
	(Some(a), a_llval)
	} else {
	assert_eq!(offset, a.size(bx.cx()).align_to(b.align(bx.cx()).abi));
	assert_eq!(field.size, b.size(bx.cx()));
	(Some(b), b_llval)
	}
	}

	_ => {
	span_bug!(fx.mir.span, "OperandRef::extract_field({:?}): not applicable", self)
	}
	};

compiler: Document the offset invariant of OperandValue::Pair #142044

compiler: Document the offset invariant of OperandValue::Pair #142044

Conversation

workingjubilee commented Jun 4, 2025

Uh oh!

rustbot commented Jun 4, 2025

Uh oh!

workingjubilee Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

scottmcm Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

scottmcm Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

scottmcm commented Jun 5, 2025

Uh oh!

bors commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

compiler: Document the offset invariant of `OperandValue::Pair` #142044

compiler: Document the offset invariant of `OperandValue::Pair` #142044