-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[cg_ssa] Eliminate the is_backend_{immediate,scalar_pair,ref} methods
#159164
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1871,12 +1871,25 @@ impl BackendRepr { | |
| } | ||
| } | ||
|
|
||
| /// Returns `true` if this is a scalar type | ||
| /// Returns `true` if this is specifically a [`Self::Scalar`] type. | ||
| /// | ||
| /// This excludes SIMD types. | ||
| #[inline] | ||
| pub fn is_scalar(&self) -> bool { | ||
| matches!(*self, BackendRepr::Scalar(_)) | ||
| } | ||
|
|
||
| /// Returns `true` if this is a scalar type or SIMD type. | ||
| #[inline] | ||
| pub fn is_scalar_or_simd(&self) -> bool { | ||
| matches!( | ||
| *self, | ||
| BackendRepr::Scalar(_) | ||
| | BackendRepr::SimdVector { .. } | ||
| | BackendRepr::SimdScalableVector { .. } | ||
| ) | ||
| } | ||
|
|
||
| /// Returns `true` if this is a bool | ||
| #[inline] | ||
| pub fn is_bool(&self) -> bool { | ||
|
|
@@ -2322,6 +2335,25 @@ impl<FieldIdx: Idx, VariantIdx: Idx> LayoutData<FieldIdx, VariantIdx> { | |
| } | ||
| } | ||
|
|
||
| /// In the backend, a value with this type and layout is fully represented by | ||
| /// its SSA value(s), independent of the contents of memory. | ||
| /// | ||
| /// For example, you can swap by reading both then writing both without using | ||
| /// an alloca because the store of one cannot affect the value of the other. | ||
| /// | ||
| /// Any projection into a standalone type must also yield a standalone type, | ||
| /// since it might not be in memory at all. | ||
| #[inline] | ||
| pub fn is_ssa_standalone(&self) -> bool { | ||
| match self.backend_repr { | ||
| BackendRepr::Memory { .. } => self.is_zst(), | ||
| BackendRepr::Scalar(..) | ||
| | BackendRepr::ScalarPair { .. } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, is a ScalarPair really an "SSA standalone"...? Hmm...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For everywhere that uses this method, 100% yes it is. Maybe a different name would make that clearer, but it definitely needs to be in the same category as For example you can swap two pairs by loading both and writing both, unlike
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah true true. 'kay! |
||
| | BackendRepr::SimdVector { .. } | ||
| | BackendRepr::SimdScalableVector { .. } => true, | ||
| } | ||
| } | ||
|
|
||
| /// Checks if these two `Layout` are equal enough to be considered "the same for all function | ||
| /// call ABIs". Note however that real ABIs depend on more details that are not reflected in the | ||
| /// `Layout`; the `PassMode` need to be compared as well. Also note that we assume | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -209,8 +209,6 @@ impl<'a, 'tcx> CodegenCx<'a, 'tcx> { | |||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| pub(crate) trait LayoutLlvmExt<'tcx> { | ||||||||||||||||||||||||||||||||||||||||
| fn is_llvm_immediate(&self) -> bool; | ||||||||||||||||||||||||||||||||||||||||
| fn is_llvm_scalar_pair(&self) -> bool; | ||||||||||||||||||||||||||||||||||||||||
| fn llvm_type<'a>(&self, cx: &CodegenCx<'a, 'tcx>) -> &'a Type; | ||||||||||||||||||||||||||||||||||||||||
| fn immediate_llvm_type<'a>(&self, cx: &CodegenCx<'a, 'tcx>) -> &'a Type; | ||||||||||||||||||||||||||||||||||||||||
| fn scalar_llvm_type_at<'a>(&self, cx: &CodegenCx<'a, 'tcx>, scalar: Scalar) -> &'a Type; | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -223,25 +221,6 @@ pub(crate) trait LayoutLlvmExt<'tcx> { | |||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| impl<'tcx> LayoutLlvmExt<'tcx> for TyAndLayout<'tcx> { | ||||||||||||||||||||||||||||||||||||||||
| fn is_llvm_immediate(&self) -> bool { | ||||||||||||||||||||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are ancient, still there in recognizable form 8+ years ago in rust/src/librustc_trans/type_of.rs Lines 195 to 213 in cdeb4b0
But also the types involved are pretty different now. Being explicitly |
||||||||||||||||||||||||||||||||||||||||
| match self.backend_repr { | ||||||||||||||||||||||||||||||||||||||||
| BackendRepr::Scalar(_) | ||||||||||||||||||||||||||||||||||||||||
| | BackendRepr::SimdVector { .. } | ||||||||||||||||||||||||||||||||||||||||
| | BackendRepr::SimdScalableVector { .. } => true, | ||||||||||||||||||||||||||||||||||||||||
| BackendRepr::ScalarPair { .. } | BackendRepr::Memory { .. } => false, | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| fn is_llvm_scalar_pair(&self) -> bool { | ||||||||||||||||||||||||||||||||||||||||
| match self.backend_repr { | ||||||||||||||||||||||||||||||||||||||||
| BackendRepr::ScalarPair { .. } => true, | ||||||||||||||||||||||||||||||||||||||||
| BackendRepr::Scalar(_) | ||||||||||||||||||||||||||||||||||||||||
| | BackendRepr::SimdVector { .. } | ||||||||||||||||||||||||||||||||||||||||
| | BackendRepr::SimdScalableVector { .. } | ||||||||||||||||||||||||||||||||||||||||
| | BackendRepr::Memory { .. } => false, | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /// Gets the LLVM type corresponding to a Rust type, i.e., `rustc_middle::ty::Ty`. | ||||||||||||||||||||||||||||||||||||||||
| /// The pointee type of the pointer in `PlaceRef` is always this type. | ||||||||||||||||||||||||||||||||||||||||
| /// For sized types, it is also the right LLVM type for an `alloca` | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,12 +83,11 @@ impl<'a, 'b, 'tcx, Bx: BuilderMethods<'b, 'tcx>> LocalAnalyzer<'a, 'b, 'tcx, Bx> | |
| LocalKind::Unused => { | ||
| let ty = fx.monomorphize(decl.ty); | ||
| let layout = fx.cx.spanned_layout_of(ty, decl.source_info.span); | ||
| *kind = | ||
| if fx.cx.is_backend_immediate(layout) || fx.cx.is_backend_scalar_pair(layout) { | ||
| LocalKind::SSA(location) | ||
| } else { | ||
| LocalKind::Memory | ||
| }; | ||
| *kind = if let abi::BackendRepr::Memory { .. } = layout.backend_repr { | ||
| LocalKind::Memory | ||
| } else { | ||
| LocalKind::SSA(location) | ||
| }; | ||
|
Comment on lines
+86
to
+90
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm realizing I'm not entirely sure why this isn't conditioned on just
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you say, with ZSTs already excluded it'll work both ways. I don't know if it's easier to think of things here as "well you can only use a SSA definition if it's standalone" or "well if it's memory then it needs to me in memory". (I guess the |
||
| } | ||
| LocalKind::SSA(_) => *kind = LocalKind::Memory, | ||
| } | ||
|
|
@@ -157,7 +156,7 @@ impl<'a, 'b, 'tcx, Bx: BuilderMethods<'b, 'tcx>> LocalAnalyzer<'a, 'b, 'tcx, Bx> | |
| } | ||
| } | ||
| debug_assert!( | ||
| !self.fx.cx.is_backend_ref(layout), | ||
| layout.is_ssa_standalone(), | ||
| "Post-projection {place_ref:?} layout should be non-Ref, but it's {layout:?}", | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -28,37 +28,54 @@ use crate::traits::*; | |||||||||
| pub enum OperandValue<V> { | ||||||||||
| /// A reference to the actual operand. The data is guaranteed | ||||||||||
| /// to be valid for the operand's lifetime. | ||||||||||
| /// The second value, if any, is the extra data (vtable or length) | ||||||||||
| /// The [`PlaceValue::llextra`], if any, is the extra data (vtable or length) | ||||||||||
| /// which indicates that it refers to an unsized rvalue. | ||||||||||
| /// | ||||||||||
| /// An `OperandValue` *must* be this variant for any type for which | ||||||||||
| /// [`LayoutTypeCodegenMethods::is_backend_ref`] returns `true`. | ||||||||||
| /// [`rustc_abi::LayoutData::is_ssa_standalone`] returns `false`. | ||||||||||
| /// (That basically amounts to "isn't one of the other variants".) | ||||||||||
| /// | ||||||||||
| /// This holds a [`PlaceValue`] (like a [`PlaceRef`] does) with a pointer | ||||||||||
| /// to the location holding the value. The type behind that pointer is the | ||||||||||
| /// one returned by [`LayoutTypeCodegenMethods::backend_type`]. | ||||||||||
| /// | ||||||||||
| /// Note that a [`load_operand`] which produces this variant didn't actually | ||||||||||
| /// *load* anything; it just put the pointer-to-place into this variant. | ||||||||||
| /// | ||||||||||
| /// [`load_operand`]: BuilderMethods::load_operand | ||||||||||
| Ref(PlaceValue<V>), | ||||||||||
| /// A single LLVM immediate value. | ||||||||||
| /// | ||||||||||
| /// An `OperandValue` *must* be this variant for any type for which | ||||||||||
| /// [`LayoutTypeCodegenMethods::is_backend_immediate`] returns `true`. | ||||||||||
| /// An `OperandValue` *must* be this variant for any type that's | ||||||||||
| /// [`BackendRepr::Scalar`], [`BackendRepr::SimdVector`], or | ||||||||||
| /// [`BackendRepr::SimdScalableVector`]. | ||||||||||
| /// | ||||||||||
| /// The backend value in this variant must be the *immediate* backend type, | ||||||||||
| /// as returned by [`LayoutTypeCodegenMethods::immediate_backend_type`]. | ||||||||||
| /// | ||||||||||
| /// Notably, that means that in LLVM a `bool` is `i1` here, even though we | ||||||||||
| /// load and store `bool`s as LLVM's `i8` type. Methods such as | ||||||||||
| /// [`BuilderMethods::load_operand`] and [`OperandRef::store_with_annotation`] | ||||||||||
| /// will handle that correctly, but if you're using the value directly or | ||||||||||
| /// implementing such methods, be sure to convert using | ||||||||||
| /// [`BuilderMethods::from_immediate`] and [`BuilderMethods::to_immediate_scalar`] | ||||||||||
| /// in the appropriate places. | ||||||||||
| Immediate(V), | ||||||||||
| /// A pair of immediate LLVM values. Used by wide pointers too. | ||||||||||
| /// A pair of immediate LLVM values. | ||||||||||
| /// | ||||||||||
| /// Notably this includes wide pointers, where the two values are the pointer | ||||||||||
| /// and the metadata (slice length, vtable pointer, etc). | ||||||||||
| /// | ||||||||||
| /// # Invariants | ||||||||||
| /// - For `Pair(a, b)`, `a` is always at offset 0, but may have `FieldIdx(1..)` | ||||||||||
| /// - `b` is not at offset 0, because `V` is not a 1ZST type. | ||||||||||
| /// - `a` and `b` will have a different FieldIdx, but otherwise `b`'s may be lower | ||||||||||
| /// or they may not be adjacent, due to arbitrary numbers of 1ZST fields that | ||||||||||
| /// will not affect the shape of the data which determines if `Pair` will be used. | ||||||||||
| /// - An `OperandValue` *must* be this variant for any type for which | ||||||||||
| /// [`LayoutTypeCodegenMethods::is_backend_scalar_pair`] returns `true`. | ||||||||||
| /// - An `OperandValue` *must* be this variant for any type that's [`BackendRepr::ScalarPair`]. | ||||||||||
| /// - The backend values in this variant must be the *immediate* backend types, | ||||||||||
| /// as returned by [`LayoutTypeCodegenMethods::scalar_pair_element_backend_type`] | ||||||||||
| /// with `immediate: true`. | ||||||||||
| /// with `immediate: true`. See the note in [`Self::Immediate`]. | ||||||||||
| Pair(V, V), | ||||||||||
| /// A value taking no bytes, and which therefore needs no LLVM value at all. | ||||||||||
| /// | ||||||||||
|
|
@@ -104,16 +121,18 @@ impl<V: CodegenObject> OperandValue<V> { | |||||||||
| } | ||||||||||
|
|
||||||||||
| #[must_use] | ||||||||||
| pub(crate) fn is_expected_variant_for_type<'tcx, Cx: LayoutTypeCodegenMethods<'tcx>>( | ||||||||||
| &self, | ||||||||||
| cx: &Cx, | ||||||||||
| ty: TyAndLayout<'tcx>, | ||||||||||
| ) -> bool { | ||||||||||
| match self { | ||||||||||
| OperandValue::ZeroSized => ty.is_zst(), | ||||||||||
| OperandValue::Immediate(_) => cx.is_backend_immediate(ty), | ||||||||||
| OperandValue::Pair(_, _) => cx.is_backend_scalar_pair(ty), | ||||||||||
| OperandValue::Ref(_) => cx.is_backend_ref(ty), | ||||||||||
| pub(crate) fn is_expected_variant_for_type<'tcx>(&self, ty: TyAndLayout<'tcx>) -> bool { | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...what a strange function. Feels worrying that it exists.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK it's only for ICEing. For example I added rust/compiler/rustc_codegen_ssa/src/mir/intrinsic.rs Lines 611 to 614 in a6050b7
when doing the "intrinsic without an alloca" work as a double-check that it's returning the correct thing for the type.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm okay but it feels very "we're not correct-by-construction", is what I'm thinking.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well yeah, the problem is that everything's just a See our previous conversation about maybe having |
||||||||||
| match (self, ty.backend_repr) { | ||||||||||
| (OperandValue::ZeroSized, BackendRepr::Memory { .. }) => ty.is_zst(), | ||||||||||
| (OperandValue::Ref(_), BackendRepr::Memory { .. }) => !ty.is_zst(), | ||||||||||
| ( | ||||||||||
| OperandValue::Immediate(_), | ||||||||||
| BackendRepr::Scalar(..) | ||||||||||
| | BackendRepr::SimdVector { .. } | ||||||||||
| | BackendRepr::SimdScalableVector { .. }, | ||||||||||
| ) => true, | ||||||||||
| (OperandValue::Pair(_, _), BackendRepr::ScalarPair { .. }) => true, | ||||||||||
| _ => false, | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
@@ -369,11 +388,11 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { | |||||||||
| let field = self.layout.field(bx.cx(), i); | ||||||||||
| let offset = self.layout.fields.offset(i); | ||||||||||
|
|
||||||||||
| if !bx.is_backend_ref(self.layout) && bx.is_backend_ref(field) { | ||||||||||
| if self.layout.is_ssa_standalone() && !field.is_ssa_standalone() { | ||||||||||
| // Part of https://github.com/rust-lang/compiler-team/issues/838 | ||||||||||
| span_bug!( | ||||||||||
| fx.mir.span, | ||||||||||
| "Non-ref type {self:?} cannot project to ref field type {field:?}", | ||||||||||
| "Standalone type {self:?} cannot project to memory-dependent field type {field:?}", | ||||||||||
| ); | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -943,18 +962,23 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue<V> { | |||||||||
| layout: TyAndLayout<'tcx>, | ||||||||||
| ) -> OperandValue<V> { | ||||||||||
| assert!(layout.is_sized()); | ||||||||||
| if layout.is_zst() { | ||||||||||
| OperandValue::ZeroSized | ||||||||||
| } else if bx.cx().is_backend_immediate(layout) { | ||||||||||
| let ibty = bx.cx().immediate_backend_type(layout); | ||||||||||
| OperandValue::Immediate(bx.const_poison(ibty)) | ||||||||||
| } else if bx.cx().is_backend_scalar_pair(layout) { | ||||||||||
| let ibty0 = bx.cx().scalar_pair_element_backend_type(layout, 0, true); | ||||||||||
| let ibty1 = bx.cx().scalar_pair_element_backend_type(layout, 1, true); | ||||||||||
| OperandValue::Pair(bx.const_poison(ibty0), bx.const_poison(ibty1)) | ||||||||||
| } else { | ||||||||||
| let ptr = bx.cx().type_ptr(); | ||||||||||
| OperandValue::Ref(PlaceValue::new_sized(bx.const_poison(ptr), layout.align.abi)) | ||||||||||
| match layout.backend_repr { | ||||||||||
| _ if layout.is_zst() => OperandValue::ZeroSized, | ||||||||||
| BackendRepr::Scalar(_) | ||||||||||
| | BackendRepr::SimdVector { .. } | ||||||||||
| | BackendRepr::SimdScalableVector { .. } => { | ||||||||||
| let ibty = bx.cx().immediate_backend_type(layout); | ||||||||||
| OperandValue::Immediate(bx.const_poison(ibty)) | ||||||||||
| } | ||||||||||
| BackendRepr::ScalarPair { .. } => { | ||||||||||
| let ibty0 = bx.cx().scalar_pair_element_backend_type(layout, 0, true); | ||||||||||
| let ibty1 = bx.cx().scalar_pair_element_backend_type(layout, 1, true); | ||||||||||
| OperandValue::Pair(bx.const_poison(ibty0), bx.const_poison(ibty1)) | ||||||||||
| } | ||||||||||
| BackendRepr::Memory { .. } => { | ||||||||||
| let ptr = bx.cx().type_ptr(); | ||||||||||
| OperandValue::Ref(PlaceValue::new_sized(bx.const_poison(ptr), layout.align.abi)) | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
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.
weird name, but I kept trying to think of a better one and couldn't. It could be
is_backend_standaloneorhas_standalone_reprbut that's about it.