Update `InterpCx::project_field` to take `FieldIdx` by scottmcm · Pull Request #142103 · rust-lang/rust (original) (raw)

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

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 6, 2025

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

The Miri subtree was changed

cc @rust-lang/miri

This comment has been minimized.

As suggested by Ralf in 142005.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR highlights several situations where we can do improvements, but let's land this on its own and investigate from_usize and as_usize calls later

return None;
}
found = Some((field_idx, field));
found = Some((FieldIdx::from_usize(field_idx), field));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be worth adding an iter and an iter_enumerated method to FieldsShape

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, most of the layout-related usizes in ABI are sketchy. I've tried pulling on the tendrils before, but usually ended up giving up because of just how far that goes and -- as I mentioned in the other PR -- that the ones related to arrays sometimes need more than FieldIdx, and figuring out which those are can be hard.

(Conveniently this one was obvious because a over-4-billion-length array clearly can't have exactly one non-ZST.)

📌 Commit 8bce225 has been approved by oli-obk

It is now in the queue for this repository.

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 6, 2025

impl FieldIdx {
/// The second field.
///
/// For use alongside [`FieldIdx::ZERO`], particularly with scalar pairs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not defined near FieldIdx::ZERO?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because all rustc_newtype_index! types get a ZERO; it's not something special for FieldIdx.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add a const ONE = 1; in the macro to get it generated.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That generates it as a non-associated constant, though -- like the START_BLOCK mentioned -- not an associated one. And because ZERO is associated, I wanted ONE to be associated as well.

let fields_iter = (0..field_count)
.map(\|i
let field_op = ecx.project_field(&down, i).discard_err()?;
let field_op = ecx.project_field(&down, FieldIdx::from_usize(i)).discard_err()?;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also use a typed field iterator


for i in 0..field_count {
let field = ecx.project_field(&place, i).unwrap();
let field = ecx.project_field(&place, FieldIdx::from_usize(i)).unwrap();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And more here

(probably not worth pointing them all out)

// Computing the layout does normalization, so we get a normalized type out of this
// even if the field type is non-normalized (possible e.g. via associated types).
let field_layout = base.layout().field(self, field);
let field_layout = base.layout().field(self, field.as_usize());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does field not take FieldIdx...?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co-authored-by: Ralf Jung post@ralfj.de

📌 Commit f9cf096 has been approved by oli-obk

It is now in the queue for this repository.

Yeah those should be separate methods for fields and array elements. Is this entire project tracked somewhere? (Also re: field iterators)

I think "project" overstates at least how I've been handling this. I had rust-lang/compiler-team#606 to add FieldIdx originally, then @WaffleLapkin's rust-lang/compiler-team#639 is explicitly about using it in more places.

There's never been a tracking issue for it, or a list of all the things that should happen, or anything. Just a bunch of "spots I saw in passing" PRs.

bors added a commit that referenced this pull request

Jun 7, 2025

Rollup of 11 pull requests

Successful merges:

#140418 (Reexport types from c_size_t in std)
#141471 (unsafe keyword docs: emphasize that an unsafe fn in a trait does not get to choose its safety contract)
#141603 (Reduce ast::ptr::P to a typedef of Box)
#142043 (Verbose suggestion to make param const)
#142086 (duduplicate more AST visitor methods)
#142103 (Update InterpCx::project_field to take FieldIdx)
#142105 (remove extraneous text)
#142112 (fix typo)
#142113 (Reduce confusion of some drop order tests)
#142114 (Compute number of digits instead of relying on constant value for u128 display code)
#142118 (rustc_lexer: typo fix + small cleanups)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request

Jun 7, 2025

Rollup merge of #142103 - scottmcm:fieldidx-in-interp, r=oli-obk

Update InterpCx::project_field to take FieldIdx

As suggested by Ralf in #142005 (comment)

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Jun 7, 2025

bors added a commit that referenced this pull request

Jun 19, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

(Of course the optimizer can clean that up, but it'd be nice if it didn't have to -- especially in debug where it doesn't run. This is like #123886, but that only handled non-simd structs -- this PR generalizes it to all non-simd ADTs.)

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request

Jun 24, 2025

bors added a commit that referenced this pull request

Jul 4, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

bors added a commit that referenced this pull request

Jul 4, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

try-job: aarch64-gnu

bors added a commit that referenced this pull request

Jul 4, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

try-job: aarch64-gnu

bors added a commit that referenced this pull request

Jul 4, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

try-job: aarch64-gnu

bors added a commit that referenced this pull request

Jul 4, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

try-job: aarch64-gnu

bors added a commit that referenced this pull request

Jul 4, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

try-job: aarch64-gnu

bors added a commit that referenced this pull request

Jul 4, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

try-job: aarch64-gnu

bors added a commit that referenced this pull request

Jul 5, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

try-job: aarch64-gnu

github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request

Jul 7, 2025

Allow enum and union literals to also create SSA values

Today, Some(x) always goes through an alloca, even in trivial cases where the niching means the constructor doesn't even change the value.

For example, <https://rust.godbolt.org/z/6KG6PqoYz>

pub fn demo(r: &i32) -> Option<&i32> {
    Some(r)
}

currently emits the IR

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  %_0 = alloca [8 x i8], align 8
  store ptr %r, ptr %_0, align 8
  %0 = load ptr, ptr %_0, align 8
  ret ptr %0
}

but with this PR it becomes just

define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr {
start:
  ret ptr %r
}

(Of course the optimizer can clean that up, but it'd be nice if it didn't have to -- especially in debug where it doesn't run. This is like rust-lang/rust#123886, but that only handled non-simd structs -- this PR generalizes it to all non-simd ADTs.)

Doing this means handing variants other than FIRST_VARIANT, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.

Other PRs that led up to this one:

try-job: aarch64-gnu

Update InterpCx::project_field to take FieldIdx by scottmcm · Pull Request #142103 · rust-lang/rust (original) (raw)

This comment has been minimized.

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

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

Update `InterpCx::project_field` to take `FieldIdx` by scottmcm · Pull Request #142103 · rust-lang/rust (original) (raw)