-
Notifications
You must be signed in to change notification settings - Fork 13.4k
CodeGen: rework Aggregate implemention for rvalue_creates_operand cases #142383
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.
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
base: master
Are you sure you want to change the base?
Conversation
Another refactor pulled out from 138759 The previous implementation I'd written here based on `index_by_increasing_offset` is complicated to follow and difficult to extend to non-structs. This changes the implementation, without actually changing any codegen (thus no test changes either), to be more like the existing `extract_field` (<https://github.com/rust-lang/rust/blob/2b0274c71dba0e24370ebf65593da450e2e91868/compiler/rustc_codegen_ssa/src/mir/operand.rs#L345-L425>) in that it allows setting a particular field directly. Notably I've found this one much easier to get right, in particular because having the `OperandRef<Result<V, Scalar>>` gives a really useful thing to include in ICE messages if something did happen to go wrong.
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_codegen_ssa |
pub(crate) fn builder(layout: TyAndLayout<'tcx>) -> OperandRef<'tcx, Result<V, abi::Scalar>> { | ||
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)), | ||
_ => bug!("Cannot use type in operand builder: {layout:?}"), | ||
}; | ||
OperandRef { val, layout } | ||
} | ||
|
||
/// Determines whether [`OperandRef::builder`] is supported for this `layout`. | ||
/// | ||
/// Only needed by [`FunctionCx::rvalue_creates_operand`], but here so the | ||
/// two `match`es are proximate in implementation. | ||
pub(crate) fn supports_builder(layout: TyAndLayout<'tcx>) -> bool { | ||
match layout.backend_repr { | ||
BackendRepr::Memory { .. } if layout.is_zst() => true, | ||
BackendRepr::Scalar(_) | BackendRepr::ScalarPair(_, _) => true, | ||
BackendRepr::Memory { .. } | BackendRepr::SimdVector { .. } => false, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I believe my main question here is why couldn't we just have a return of Option<OperandRef<...>>
to prevent them from getting out of sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously that requires skipping the bug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I made them two because they're called in such different places. I guess it's cheap enough to just call it even if I don't actually want the result, just the .is_ok()
and I can unwrap
in the other one...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, and even if supports_builder
is a separate function it will probably generate the code just fine if it relies on LLVM optimizing builder(layout).is_ok()
or something.
A non-trivial refactor pulled out from #138759
r? workingjubilee
The previous implementation I'd written here based on
index_by_increasing_offset
is complicated to follow and difficult to extend to non-structs.This changes the implementation, without actually changing any codegen (thus no test changes either), to be more like the existing
extract_field
(rust/compiler/rustc_codegen_ssa/src/mir/operand.rs
Lines 345 to 425 in 2b0274c
Notably I've found this one much easier to get right, in particular because having the
OperandRef<Result<V, Scalar>>
gives a really useful thing to include in ICE messages if something did happen to go wrong.