Update `InterpCx::project_field` to take `FieldIdx`#142103

scottmcm · 2025-06-06T02:02:35Z

As suggested by Ralf in #142005 (comment)

rustbot · 2025-06-06T02:02:40Z

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 · 2025-06-06T02:02:42Z

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

As suggested by Ralf in 142005.

oli-obk

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

oli-obk · 2025-06-06T08:00:45Z

compiler/rustc_abi/src/layout/ty.rs

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


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

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

oli-obk · 2025-06-06T08:06:59Z

@bors r+ rollup

bors · 2025-06-06T08:07:01Z

📌 Commit 8bce225 has been approved by oli-obk

It is now in the queue for this repository.

compiler/rustc_abi/src/layout/ty.rs

RalfJung · 2025-06-06T09:18:48Z

compiler/rustc_abi/src/layout/ty.rs

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


Why is this not defined near FieldIdx::ZERO?

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

(See for example how there's both https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.BasicBlock.html#associatedconstant.ZERO and https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/constant.START_BLOCK.html.)

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

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.

RalfJung · 2025-06-06T09:19:24Z

compiler/rustc_const_eval/src/const_eval/mod.rs

    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()?;


This could also use a typed field iterator

RalfJung · 2025-06-06T09:19:59Z

compiler/rustc_const_eval/src/const_eval/valtrees.rs


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


And more here

(probably not worth pointing them all out)

RalfJung · 2025-06-06T09:20:51Z

compiler/rustc_const_eval/src/interpret/projection.rs

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


Why does field not take FieldIdx...?

It's https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.TyAndLayout.html#method.field, I think. And that's like I mentioned in #142005 (comment) something that takes usize today, because it works on arrays https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_middle/ty/layout.rs.html#795-998.

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

scottmcm · 2025-06-06T09:31:00Z

@bors r=oli-obk

bors · 2025-06-06T09:31:02Z

📌 Commit f9cf096 has been approved by oli-obk

It is now in the queue for this repository.

RalfJung · 2025-06-06T10:28:08Z

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

scottmcm · 2025-06-06T17:44:35Z

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.

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

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)

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

WaffleLapkin · 2025-06-08T11:49:44Z

@WaffleLapkin's rust-lang/compiler-team#639 is explicitly about using it in more places.

Oh. Did I mean to work on it and then forgot? 😓

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> ```rust pub fn demo(r: &i32) -> Option<&i32> { Some(r) } ``` currently emits the IR ```llvm 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 ```llvm 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 `struct`s -- 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: - #142005 - #142103 - #142324 - #142383

…-obk Update `InterpCx::project_field` to take `FieldIdx` As suggested by Ralf in rust-lang#142005 (comment)

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> ```rust pub fn demo(r: &i32) -> Option<&i32> { Some(r) } ``` currently emits the IR ```llvm 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 ```llvm 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 `struct`s -- 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: - #142005 - #142103 - #142324 - #142383

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> ```rust pub fn demo(r: &i32) -> Option<&i32> { Some(r) } ``` currently emits the IR ```llvm 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 ```llvm 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 `struct`s -- 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: - #142005 - #142103 - #142324 - #142383 --- try-job: aarch64-gnu

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> ```rust pub fn demo(r: &i32) -> Option<&i32> { Some(r) } ``` currently emits the IR ```llvm 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 ```llvm 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 `struct`s -- 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: - rust-lang/rust#142005 - rust-lang/rust#142103 - rust-lang/rust#142324 - rust-lang/rust#142383 --- try-job: aarch64-gnu

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

rustbot assigned oli-obk Jun 6, 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 6, 2025

This comment has been minimized.

Sign in to view

Update InterpCx::project_field to take FieldIdx

8bce225

As suggested by Ralf in 142005.

scottmcm force-pushed the fieldidx-in-interp branch from 74c269e to 8bce225 Compare June 6, 2025 02:16

oli-obk approved these changes Jun 6, 2025

View reviewed changes

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

RalfJung reviewed Jun 6, 2025

View reviewed changes

compiler/rustc_abi/src/layout/ty.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jun 6, 2025

View reviewed changes

Update compiler/rustc_abi/src/layout/ty.rs

f9cf096

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

GuillaumeGomez mentioned this pull request Jun 6, 2025

Rollup of 11 pull requests #142133

Merged

bors merged commit 3a6f1b0 into rust-lang:master Jun 7, 2025
10 checks passed

rustbot added this to the 1.89.0 milestone Jun 7, 2025

rust-timer added a commit that referenced this pull request Jun 7, 2025

Unrolled build for #142103

7e2ca66

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)

scottmcm deleted the fieldidx-in-interp branch June 7, 2025 22:58

scottmcm mentioned this pull request Jun 19, 2025

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

Merged

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jun 24, 2025

Rollup merge of rust-lang#142103 - scottmcm:fieldidx-in-interp, r=oli…

3630039

…-obk Update `InterpCx::project_field` to take `FieldIdx` As suggested by Ralf in rust-lang#142005 (comment)

Uh oh!

Conversation

scottmcm commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 6, 2025

Uh oh!

rustbot commented Jun 6, 2025

Uh oh!

This comment has been minimized.

oli-obk left a comment

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!

oli-obk commented Jun 6, 2025

Uh oh!

bors commented Jun 6, 2025

Uh oh!

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!

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!

scottmcm commented Jun 6, 2025

Uh oh!

bors commented Jun 6, 2025

Uh oh!

RalfJung commented Jun 6, 2025 via email

Uh oh!

scottmcm commented Jun 6, 2025

Uh oh!

Uh oh!

WaffleLapkin commented Jun 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

scottmcm commented Jun 6, 2025 •

edited

Loading