Skip to content
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

Use niche-filling optimization even when multiple variants have data. #94075

Merged
merged 2 commits into from
Sep 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion compiler/rustc_ast/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3075,7 +3075,8 @@ mod size_asserts {
static_assert_size!(Block, 48);
static_assert_size!(Expr, 104);
static_assert_size!(ExprKind, 72);
static_assert_size!(Fn, 192);
#[cfg(not(bootstrap))]
static_assert_size!(Fn, 184);
static_assert_size!(ForeignItem, 96);
static_assert_size!(ForeignItemKind, 24);
static_assert_size!(GenericArg, 24);
Expand Down
11 changes: 6 additions & 5 deletions compiler/rustc_codegen_cranelift/src/discriminant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ pub(crate) fn codegen_set_discriminant<'tcx>(
Variants::Multiple {
tag: _,
tag_field,
tag_encoding: TagEncoding::Niche { dataful_variant, ref niche_variants, niche_start },
tag_encoding: TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start },
variants: _,
} => {
if variant_index != dataful_variant {
if variant_index != untagged_variant {
let niche = place.place_field(fx, mir::Field::new(tag_field));
let niche_value = variant_index.as_u32() - niche_variants.start().as_u32();
let niche_value = ty::ScalarInt::try_from_uint(
Expand Down Expand Up @@ -113,7 +113,7 @@ pub(crate) fn codegen_get_discriminant<'tcx>(
let res = CValue::by_val(val, dest_layout);
dest.write_cvalue(fx, res);
}
TagEncoding::Niche { dataful_variant, ref niche_variants, niche_start } => {
TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start } => {
// Rebase from niche values to discriminants, and check
// whether the result is in range for the niche variants.

Expand Down Expand Up @@ -169,8 +169,9 @@ pub(crate) fn codegen_get_discriminant<'tcx>(
fx.bcx.ins().iadd_imm(relative_discr, i64::from(niche_variants.start().as_u32()))
};

let dataful_variant = fx.bcx.ins().iconst(cast_to, i64::from(dataful_variant.as_u32()));
let discr = fx.bcx.ins().select(is_niche, niche_discr, dataful_variant);
let untagged_variant =
fx.bcx.ins().iconst(cast_to, i64::from(untagged_variant.as_u32()));
let discr = fx.bcx.ins().select(is_niche, niche_discr, untagged_variant);
let res = CValue::by_val(discr, dest_layout);
dest.write_cvalue(fx, res);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ const SINGLE_VARIANT_VIRTUAL_DISR: u64 = 0;
/// compiler versions.
///
/// Niche-tag enums have one special variant, usually called the
/// "dataful variant". This variant has a field that
/// "untagged variant". This variant has a field that
/// doubles as the tag of the enum. The variant is active when the value of
/// that field is within a pre-defined range. Therefore the variant struct
/// has a `DISCR_BEGIN` and `DISCR_END` field instead of `DISCR_EXACT` in
Expand Down Expand Up @@ -249,7 +249,7 @@ pub(super) fn build_enum_type_di_node<'ll, 'tcx>(
None,
),
Variants::Multiple {
tag_encoding: TagEncoding::Niche { dataful_variant, .. },
tag_encoding: TagEncoding::Niche { untagged_variant, .. },
ref variants,
tag_field,
..
Expand All @@ -260,7 +260,7 @@ pub(super) fn build_enum_type_di_node<'ll, 'tcx>(
enum_type_di_node,
variants.indices(),
tag_field,
Some(dataful_variant),
Some(untagged_variant),
),
}
},
Expand Down Expand Up @@ -391,7 +391,7 @@ fn build_union_fields_for_enum<'ll, 'tcx>(
enum_type_di_node: &'ll DIType,
variant_indices: impl Iterator<Item = VariantIdx> + Clone,
tag_field: usize,
dataful_variant_index: Option<VariantIdx>,
untagged_variant_index: Option<VariantIdx>,
) -> SmallVec<&'ll DIType> {
let tag_base_type = super::tag_base_type(cx, enum_type_and_layout);

Expand Down Expand Up @@ -436,7 +436,7 @@ fn build_union_fields_for_enum<'ll, 'tcx>(
variant_names_type_di_node,
tag_base_type,
tag_field,
dataful_variant_index,
untagged_variant_index,
)
}

Expand Down Expand Up @@ -472,7 +472,7 @@ fn build_variant_struct_wrapper_type_di_node<'ll, 'tcx>(
enum_or_generator_type_and_layout: TyAndLayout<'tcx>,
enum_or_generator_type_di_node: &'ll DIType,
variant_index: VariantIdx,
dataful_variant_index: Option<VariantIdx>,
untagged_variant_index: Option<VariantIdx>,
variant_struct_type_di_node: &'ll DIType,
variant_names_type_di_node: &'ll DIType,
tag_base_type_di_node: &'ll DIType,
Expand Down Expand Up @@ -517,7 +517,7 @@ fn build_variant_struct_wrapper_type_di_node<'ll, 'tcx>(
}
}
DiscrResult::Range(min, max) => {
assert_eq!(Some(variant_index), dataful_variant_index);
assert_eq!(Some(variant_index), untagged_variant_index);
if is_128_bits {
DiscrKind::Range128(min, max)
} else {
Expand Down Expand Up @@ -757,7 +757,7 @@ fn build_union_fields_for_direct_tag_enum_or_generator<'ll, 'tcx>(
discr_type_di_node: &'ll DIType,
tag_base_type: Ty<'tcx>,
tag_field: usize,
dataful_variant_index: Option<VariantIdx>,
untagged_variant_index: Option<VariantIdx>,
) -> SmallVec<&'ll DIType> {
let tag_base_type_di_node = type_di_node(cx, tag_base_type);
let mut unions_fields = SmallVec::with_capacity(variant_field_infos.len() + 1);
Expand All @@ -776,7 +776,7 @@ fn build_union_fields_for_direct_tag_enum_or_generator<'ll, 'tcx>(
enum_type_and_layout,
enum_type_di_node,
variant_member_info.variant_index,
dataful_variant_index,
untagged_variant_index,
variant_member_info.variant_struct_type_di_node,
discr_type_di_node,
tag_base_type_di_node,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ impl DiscrResult {
/// Returns the discriminant value corresponding to the variant index.
///
/// Will return `None` if there is less than two variants (because then the enum won't have)
/// a tag, and if this is the dataful variant of a niche-layout enum (because then there is no
/// a tag, and if this is the untagged variant of a niche-layout enum (because then there is no
/// single discriminant value).
fn compute_discriminant_value<'ll, 'tcx>(
cx: &CodegenCx<'ll, 'tcx>,
Expand All @@ -430,11 +430,11 @@ fn compute_discriminant_value<'ll, 'tcx>(
enum_type_and_layout.ty.discriminant_for_variant(cx.tcx, variant_index).unwrap().val,
),
&Variants::Multiple {
tag_encoding: TagEncoding::Niche { ref niche_variants, niche_start, dataful_variant },
tag_encoding: TagEncoding::Niche { ref niche_variants, niche_start, untagged_variant },
tag,
..
} => {
if variant_index == dataful_variant {
if variant_index == untagged_variant {
let valid_range = enum_type_and_layout
.for_variant(cx, variant_index)
.largest_niche
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ fn build_discr_member_di_node<'ll, 'tcx>(
///
/// The DW_AT_discr_value is optional, and is omitted if
/// - This is the only variant of a univariant enum (i.e. their is no discriminant)
/// - This is the "dataful" variant of a niche-layout enum
/// - This is the "untagged" variant of a niche-layout enum
/// (where only the other variants are identified by a single value)
///
/// There is only ever a single member, the type of which is a struct that describes the
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_codegen_ssa/src/mir/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
};
bx.intcast(tag.immediate(), cast_to, signed)
}
TagEncoding::Niche { dataful_variant, ref niche_variants, niche_start } => {
TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start } => {
// Rebase from niche values to discriminants, and check
// whether the result is in range for the niche variants.
let niche_llty = bx.cx().immediate_backend_type(tag.layout);
Expand Down Expand Up @@ -302,7 +302,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
bx.select(
is_niche,
niche_discr,
bx.cx().const_uint(cast_to, dataful_variant.as_u32() as u64),
bx.cx().const_uint(cast_to, untagged_variant.as_u32() as u64),
)
}
}
Expand Down Expand Up @@ -337,11 +337,11 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
}
Variants::Multiple {
tag_encoding:
TagEncoding::Niche { dataful_variant, ref niche_variants, niche_start },
TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start },
tag_field,
..
} => {
if variant_index != dataful_variant {
if variant_index != untagged_variant {
let niche = self.project_field(bx, tag_field);
let niche_llty = bx.cx().immediate_backend_type(niche.layout);
let niche_value = variant_index.as_u32() - niche_variants.start().as_u32();
Expand Down
16 changes: 8 additions & 8 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Return the cast value, and the index.
(discr_val, index.0)
}
TagEncoding::Niche { dataful_variant, ref niche_variants, niche_start } => {
TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start } => {
let tag_val = tag_val.to_scalar();
// Compute the variant this niche value/"tag" corresponds to. With niche layout,
// discriminant (encoded in niche/tag) and variant index are the same.
Expand All @@ -736,7 +736,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
if !ptr_valid {
throw_ub!(InvalidTag(dbg_val))
}
dataful_variant
untagged_variant
}
Ok(tag_bits) => {
let tag_bits = tag_bits.assert_bits(tag_layout.size);
Expand Down Expand Up @@ -766,7 +766,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
assert!(usize::try_from(variant_index).unwrap() < variants_len);
VariantIdx::from_u32(variant_index)
} else {
dataful_variant
untagged_variant
}
}
};
Expand All @@ -780,13 +780,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}

// Some nodes are used a lot. Make sure they don't unintentionally get bigger.
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64", not(bootstrap)))]
mod size_asserts {
use super::*;
use rustc_data_structures::static_assert_size;
// These are in alphabetical order, which is easy to maintain.
static_assert_size!(Immediate, 56);
static_assert_size!(ImmTy<'_>, 72);
static_assert_size!(Operand, 64);
static_assert_size!(OpTy<'_>, 88);
static_assert_size!(Immediate, 48);
static_assert_size!(ImmTy<'_>, 64);
static_assert_size!(Operand, 56);
static_assert_size!(OpTy<'_>, 80);
}
10 changes: 6 additions & 4 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -817,15 +817,15 @@ where
}
abi::Variants::Multiple {
tag_encoding:
TagEncoding::Niche { dataful_variant, ref niche_variants, niche_start },
TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start },
tag: tag_layout,
tag_field,
..
} => {
// No need to validate that the discriminant here because the
// `TyAndLayout::for_variant()` call earlier already checks the variant is valid.

if variant_index != dataful_variant {
if variant_index != untagged_variant {
let variants_start = niche_variants.start().as_u32();
let variant_index_relative = variant_index
.as_u32()
Expand Down Expand Up @@ -890,6 +890,8 @@ mod size_asserts {
static_assert_size!(MemPlaceMeta, 24);
static_assert_size!(MemPlace, 40);
static_assert_size!(MPlaceTy<'_>, 64);
static_assert_size!(Place, 48);
static_assert_size!(PlaceTy<'_>, 72);
#[cfg(not(bootstrap))]
static_assert_size!(Place, 40);
#[cfg(not(bootstrap))]
static_assert_size!(PlaceTy<'_>, 64);
}
4 changes: 4 additions & 0 deletions compiler/rustc_data_structures/src/obligation_forest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ pub trait ObligationProcessor {
}

/// The result type used by `process_obligation`.
// `repr(C)` to inhibit the niche filling optimization. Otherwise, the `match` appearing
// in `process_obligations` is significantly slower, which can substantially affect
// benchmarks like `rustc-perf`'s inflate and keccak.
#[repr(C)]
Copy link
Contributor

Choose a reason for hiding this comment

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

#94075 (comment) makes a little confusion to me.

There were too many valid values in the niche for my MirPass to apply, and the SwitchInt was just being hit repeatedly.

Do you mean that process_obligations is slow when together with the MirPass outside this PR, or we need this repr(C) no matter if we have a MirPass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

process_obligations was slow without the MirPass. The MirPass was an attempt to speed back up some of these matches, but since it could only be applied to non-generic code, it didn't optimize process_obligations.

Actually I guess it might be worthwhile for me to do a few more benchmarks and see whether this is still slower without the repr(C). And I should probably edit this comment to not mention AddNicheCases, which no longer exists and which will confuse anyone reading this.

I'll get to those tasks in the near future.

#[derive(Debug)]
pub enum ProcessResult<O, E> {
Unchanged,
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ pub type PResult<'a, T> = Result<T, DiagnosticBuilder<'a, ErrorGuaranteed>>;
// (See also the comment on `DiagnosticBuilder`'s `diagnostic` field.)
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(PResult<'_, ()>, 16);
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(PResult<'_, bool>, 24);
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64", not(bootstrap)))]
rustc_data_structures::static_assert_size!(PResult<'_, bool>, 16);

#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash, Encodable, Decodable)]
pub enum SuggestionStyle {
Expand Down
15 changes: 10 additions & 5 deletions compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3473,12 +3473,15 @@ mod size_asserts {
static_assert_size!(FnDecl<'_>, 40);
static_assert_size!(ForeignItem<'_>, 72);
static_assert_size!(ForeignItemKind<'_>, 40);
static_assert_size!(GenericArg<'_>, 40);
#[cfg(not(bootstrap))]
static_assert_size!(GenericArg<'_>, 32);
static_assert_size!(GenericBound<'_>, 48);
static_assert_size!(Generics<'_>, 56);
static_assert_size!(Impl<'_>, 80);
static_assert_size!(ImplItem<'_>, 88);
static_assert_size!(ImplItemKind<'_>, 40);
#[cfg(not(bootstrap))]
static_assert_size!(ImplItem<'_>, 80);
#[cfg(not(bootstrap))]
static_assert_size!(ImplItemKind<'_>, 32);
static_assert_size!(Item<'_>, 80);
static_assert_size!(ItemKind<'_>, 48);
static_assert_size!(Local<'_>, 64);
Expand All @@ -3490,8 +3493,10 @@ mod size_asserts {
static_assert_size!(QPath<'_>, 24);
static_assert_size!(Stmt<'_>, 32);
static_assert_size!(StmtKind<'_>, 16);
static_assert_size!(TraitItem<'_>, 96);
static_assert_size!(TraitItemKind<'_>, 56);
#[cfg(not(bootstrap))]
static_assert_size!(TraitItem<'static>, 88);
#[cfg(not(bootstrap))]
static_assert_size!(TraitItemKind<'_>, 48);
static_assert_size!(Ty<'_>, 72);
static_assert_size!(TyKind<'_>, 56);
}
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,8 @@ pub enum BinOp {
mod size_asserts {
use super::*;
// These are in alphabetical order, which is easy to maintain.
static_assert_size!(AggregateKind<'_>, 48);
#[cfg(not(bootstrap))]
static_assert_size!(AggregateKind<'_>, 40);
static_assert_size!(Operand<'_>, 24);
static_assert_size!(Place<'_>, 16);
static_assert_size!(PlaceElem<'_>, 24);
Expand Down
12 changes: 8 additions & 4 deletions compiler/rustc_middle/src/thir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -825,8 +825,12 @@ mod size_asserts {
static_assert_size!(Block, 56);
static_assert_size!(Expr<'_>, 64);
static_assert_size!(ExprKind<'_>, 40);
static_assert_size!(Pat<'_>, 72);
static_assert_size!(PatKind<'_>, 56);
static_assert_size!(Stmt<'_>, 56);
static_assert_size!(StmtKind<'_>, 48);
#[cfg(not(bootstrap))]
static_assert_size!(Pat<'_>, 64);
#[cfg(not(bootstrap))]
static_assert_size!(PatKind<'_>, 48);
#[cfg(not(bootstrap))]
static_assert_size!(Stmt<'_>, 48);
#[cfg(not(bootstrap))]
static_assert_size!(StmtKind<'_>, 40);
}
Loading