Skip to content

codegen_ssa: replace a Result by an Either #143291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 5, 2025
+24 −23
Merged

codegen_ssa: replace a Result by an Either #143291

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 24 additions & 23 deletions compiler/rustc_codegen_ssa/src/mir/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::fmt;

use itertools::Either;
use rustc_abi as abi;
use rustc_abi::{
Align, BackendRepr, FIRST_VARIANT, FieldIdx, Primitive, Size, TagEncoding, VariantIdx, Variants,
Expand Down Expand Up @@ -567,12 +568,12 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {

/// Creates an incomplete operand containing the [`abi::Scalar`]s expected based
/// on the `layout` passed. This is for use with [`OperandRef::insert_field`]
/// later to set the necessary immediate(s).
/// later to set the necessary immediate(s), one-by-one converting all the `Right` to `Left`.
///
/// Returns `None` for `layout`s which cannot be built this way.
pub(crate) fn builder(
layout: TyAndLayout<'tcx>,
) -> Option<OperandRef<'tcx, Result<V, abi::Scalar>>> {
) -> Option<OperandRef<'tcx, Either<V, abi::Scalar>>> {
// Uninhabited types are weird, because for example `Result<!, !>`
// shows up as `FieldsShape::Primitive` and we need to be able to write
// a field into `(u32, !)`. We'll do that in an `alloca` instead.
Expand All @@ -582,15 +583,15 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {

let val = match layout.backend_repr {
BackendRepr::Memory { .. } if layout.is_zst() => OperandValue::ZeroSized,
BackendRepr::Scalar(s) => OperandValue::Immediate(Err(s)),
BackendRepr::ScalarPair(a, b) => OperandValue::Pair(Err(a), Err(b)),
BackendRepr::Scalar(s) => OperandValue::Immediate(Either::Right(s)),
BackendRepr::ScalarPair(a, b) => OperandValue::Pair(Either::Right(a), Either::Right(b)),
BackendRepr::Memory { .. } | BackendRepr::SimdVector { .. } => return None,
};
Some(OperandRef { val, layout })
}
}

impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Result<V, abi::Scalar>> {
impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Either<V, abi::Scalar>> {
pub(crate) fn insert_field<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
&mut self,
bx: &mut Bx,
Expand All @@ -614,29 +615,29 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Result<V, abi::Scalar>> {
(field_layout.is_zst(), field_offset == Size::ZERO)
};

let mut update = |tgt: &mut Result<V, abi::Scalar>, src, from_scalar| {
let to_scalar = tgt.unwrap_err();
let mut update = |tgt: &mut Either<V, abi::Scalar>, src, from_scalar| {
let to_scalar = tgt.unwrap_right();
let imm = transmute_scalar(bx, src, from_scalar, to_scalar);
*tgt = Ok(imm);
*tgt = Either::Left(imm);
};

match (operand.val, operand.layout.backend_repr) {
(OperandValue::ZeroSized, _) if expect_zst => {}
(OperandValue::Immediate(v), BackendRepr::Scalar(from_scalar)) => match &mut self.val {
OperandValue::Immediate(val @ Err(_)) if is_zero_offset => {
OperandValue::Immediate(val @ Either::Right(_)) if is_zero_offset => {
update(val, v, from_scalar);
}
OperandValue::Pair(fst @ Err(_), _) if is_zero_offset => {
OperandValue::Pair(fst @ Either::Right(_), _) if is_zero_offset => {
update(fst, v, from_scalar);
}
OperandValue::Pair(_, snd @ Err(_)) if !is_zero_offset => {
OperandValue::Pair(_, snd @ Either::Right(_)) if !is_zero_offset => {
update(snd, v, from_scalar);
}
_ => bug!("Tried to insert {operand:?} into {v:?}.{f:?} of {self:?}"),
},
(OperandValue::Pair(a, b), BackendRepr::ScalarPair(from_sa, from_sb)) => {
match &mut self.val {
OperandValue::Pair(fst @ Err(_), snd @ Err(_)) => {
OperandValue::Pair(fst @ Either::Right(_), snd @ Either::Right(_)) => {
update(fst, a, from_sa);
update(snd, b, from_sb);
}
Expand All @@ -656,21 +657,21 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Result<V, abi::Scalar>> {
let field_offset = self.layout.fields.offset(f.as_usize());
let is_zero_offset = field_offset == Size::ZERO;
match &mut self.val {
OperandValue::Immediate(val @ Err(_)) if is_zero_offset => {
*val = Ok(imm);
OperandValue::Immediate(val @ Either::Right(_)) if is_zero_offset => {
*val = Either::Left(imm);
}
OperandValue::Pair(fst @ Err(_), _) if is_zero_offset => {
*fst = Ok(imm);
OperandValue::Pair(fst @ Either::Right(_), _) if is_zero_offset => {
*fst = Either::Left(imm);
}
OperandValue::Pair(_, snd @ Err(_)) if !is_zero_offset => {
*snd = Ok(imm);
OperandValue::Pair(_, snd @ Either::Right(_)) if !is_zero_offset => {
*snd = Either::Left(imm);
}
_ => bug!("Tried to insert {imm:?} into field {f:?} of {self:?}"),
}
}

/// After having set all necessary fields, this converts the
/// `OperandValue<Result<V, _>>` (as obtained from [`OperandRef::builder`])
/// `OperandValue<Either<V, _>>` (as obtained from [`OperandRef::builder`])
/// to the normal `OperandValue<V>`.
///
/// ICEs if any required fields were not set.
Expand All @@ -681,13 +682,13 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, Result<V, abi::Scalar>> {
// payload scalar will not actually have been set, so this converts
// unset scalars to corresponding `undef` values so long as the scalar
// from the layout allows uninit.
let unwrap = |r: Result<V, abi::Scalar>| match r {
Ok(v) => v,
Err(s) if s.is_uninit_valid() => {
let unwrap = |r: Either<V, abi::Scalar>| match r {
Either::Left(v) => v,
Either::Right(s) if s.is_uninit_valid() => {
let bty = cx.type_from_scalar(s);
cx.const_undef(bty)
}
Err(_) => bug!("OperandRef::build called while fields are missing {self:?}"),
Copy link
Member

Choose a reason for hiding this comment

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

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 :/

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

#138759 landed, and I rebased :)

Either::Right(_) => bug!("OperandRef::build called while fields are missing {self:?}"),
};

let val = match val {
Expand Down
Loading