codegen_ssa: replace a Result by an Either#143291

RalfJung · 2025-07-01T17:22:32Z

Err here clearly does not indicate an "error" of any sort, so the use of Result is confusing. Let's use a sum type that does not come with the baggage of Result.

rustbot · 2025-07-01T17:22:36Z

r? @saethlin

rustbot has assigned @saethlin.
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-07-01T17:22:38Z

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

workingjubilee · 2025-07-01T17:30:02Z

r? @scottmcm

scottmcm · 2025-07-02T03:55:32Z

compiler/rustc_codegen_ssa/src/mir/operand.rs


-        let unwrap = |r: Result<V, abi::Scalar>| match r {
-            Ok(v) => v,
-            Err(_) => bug!("OperandRef::build called while fields are missing {self:?}"),


I'd used an Err here because in build it is an error.

An Either is fine too, so I don't mind doing this, but I'd really rather land #138759 that will conflict with this first, since it's approaching 4 months :/

I was mostly looking at the big match in insert_field, which looked like it was... finding errors and removing them? That doesn't look like a Result.

Some actual comments explaining what happens here with that sum type would have helped a lot but, not understanding the code I can't add those unfortunately. Would be great if you could. :)

An Either is fine too, so I don't mind doing this, but I'd really rather land #138759 that will conflict with this first, since it's approaching 4 months :/

Sure, this can wait.

#138759 landed, and I rebased :)

bors · 2025-07-05T05:43:27Z

☔ The latest upstream changes (presumably #138759) made this pull request unmergeable. Please resolve the merge conflicts.

scottmcm · 2025-07-05T08:07:31Z

Thanks for rebasing! The code change looks good to me
@bors r+

bors · 2025-07-05T08:07:33Z

📌 Commit 1628a7c has been approved by scottmcm

It is now in the queue for this repository.

bors · 2025-07-05T13:40:45Z

☔ The latest upstream changes (presumably #143473) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2025-07-05T13:57:07Z

@bors r=scottmcm rollup

bors · 2025-07-05T13:57:09Z

📌 Commit 3cd13f7 has been approved by scottmcm

It is now in the queue for this repository.

Rollup of 3 pull requests Successful merges: - #143291 (codegen_ssa: replace a Result by an Either) - #143445 (move `va_copy`, `va_arg` and `va_end` to `core::intrinsics`) - #143447 ([COMPILETEST-UNTANGLE 4/N] Improve compiletest config documentation) r? `@ghost` `@rustbot` modify labels: rollup

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s Draft for now because it's built on Ralf's #143291 Inspired by #138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in #143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

Rollup merge of #143291 - RalfJung:result-isnt-either, r=scottmcm codegen_ssa: replace a Result by an Either `Err` here clearly does not indicate an "error" of any sort, so the use of `Result` is confusing. Let's use a sum type that does not come with the baggage of `Result`.

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s ~~Draft for now because it's built on Ralf's #143291~~ Inspired by #138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in #143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

Let `rvalue_creates_operand` return true for *all* `Rvalue::Aggregate`s ~~Draft for now because it's built on Ralf's rust-lang/rust#143291~~ Inspired by rust-lang/rust#138759 (comment) where I noticed that we were nearly at this point, plus the comments I was writing in rust-lang/rust#143410 that reminded me a type-dependent `true` is fine. This PR splits the `OperandRef::builder` logic out to a separate type, with the updates needed to handle SIMD as well. In doing so, that makes the existing `Aggregate` path in `codegen_rvalue_operand` capable of handing SIMD values just fine. As a result, we no longer need to do layout calculations for aggregate result types when running the analysis to determine which things can be SSA in codegen.

rustbot assigned saethlin Jul 1, 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 Jul 1, 2025

rustbot assigned scottmcm and unassigned saethlin Jul 1, 2025

scottmcm reviewed Jul 2, 2025

View reviewed changes

RalfJung force-pushed the result-isnt-either branch from 5cea731 to 1628a7c Compare July 5, 2025 07:06

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 Jul 5, 2025

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 5, 2025

RalfJung force-pushed the result-isnt-either branch from 1628a7c to 3fd85b3 Compare July 5, 2025 13:53

codegen_ssa: replace a Result by an Either

3cd13f7

RalfJung force-pushed the result-isnt-either branch from 3fd85b3 to 3cd13f7 Compare July 5, 2025 13:54

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 5, 2025

saethlin mentioned this pull request Jul 5, 2025

Rollup of 3 pull requests #143496

Merged

scottmcm mentioned this pull request Jul 5, 2025

Let rvalue_creates_operand return true for *all* Rvalue::Aggregates #143502

Merged

bors merged commit 61bdd11 into rust-lang:master Jul 5, 2025
11 checks passed

rustbot added this to the 1.90.0 milestone Jul 5, 2025

RalfJung deleted the result-isnt-either branch July 6, 2025 13:31

Uh oh!

Conversation

RalfJung commented Jul 1, 2025

Uh oh!

rustbot commented Jul 1, 2025

Uh oh!

rustbot commented Jul 1, 2025

Uh oh!

workingjubilee commented Jul 1, 2025

Uh oh!

scottmcm Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJung Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJung Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

bors commented Jul 5, 2025

Uh oh!

scottmcm commented Jul 5, 2025

Uh oh!

bors commented Jul 5, 2025

Uh oh!

bors commented Jul 5, 2025

Uh oh!

RalfJung commented Jul 5, 2025

Uh oh!

bors commented Jul 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants