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.

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

Jump to bottom

Do not attempt to commute comparison and cast to codegen discriminants #110197

Merged
merged 5 commits into from
Apr 15, 2023
Merged

Do not attempt to commute comparison and cast to codegen discriminants #110197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
65c334e
Fortify tests againts mir-opts.
cjgillot Apr 11, 2023
194497a
Remove attempt to optimize codegen for discriminants.
cjgillot Apr 10, 2023
df9342a
Remove from cranelift too.
cjgillot Apr 11, 2023
700084a
Update codegen test.
cjgillot Apr 11, 2023
a8857d2
Pacify tidy.
cjgillot Apr 13, 2023
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
89 changes: 0 additions & 89 deletions compiler/rustc_codegen_cranelift/src/discriminant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ pub(crate) fn codegen_get_discriminant<'tcx>(
}
};

let cast_to_size = dest_layout.layout.size();
let cast_to = fx.clif_type(dest_layout.ty).unwrap();

// Read the tag/niche-encoded discriminant from memory.
Expand All @@ -122,21 +121,7 @@ pub(crate) fn codegen_get_discriminant<'tcx>(
dest.write_cvalue(fx, res);
}
TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start } => {
let tag_size = tag_scalar.size(fx);
let max_unsigned = tag_size.unsigned_int_max();
let max_signed = tag_size.signed_int_max() as u128;
let min_signed = max_signed + 1;
let relative_max = niche_variants.end().as_u32() - niche_variants.start().as_u32();
let niche_end = niche_start.wrapping_add(relative_max as u128) & max_unsigned;
let range = tag_scalar.valid_range(fx);

let sle = |lhs: u128, rhs: u128| -> bool {
// Signed and unsigned comparisons give the same results,
// except that in signed comparisons an integer with the
// sign bit set is less than one with the sign bit clear.
// Toggle the sign bit to do a signed comparison.
(lhs ^ min_signed) <= (rhs ^ min_signed)
};

// We have a subrange `niche_start..=niche_end` inside `range`.
// If the value of the tag is inside this subrange, it's a
Expand All @@ -153,45 +138,6 @@ pub(crate) fn codegen_get_discriminant<'tcx>(
// }
// However, we will likely be able to emit simpler code.

// Find the least and greatest values in `range`, considered
// both as signed and unsigned.
let (low_unsigned, high_unsigned) =
if range.start <= range.end { (range.start, range.end) } else { (0, max_unsigned) };
let (low_signed, high_signed) = if sle(range.start, range.end) {
(range.start, range.end)
} else {
(min_signed, max_signed)
};

let niches_ule = niche_start <= niche_end;
let niches_sle = sle(niche_start, niche_end);
let cast_smaller = cast_to_size <= tag_size;

// In the algorithm above, we can change
// cast(relative_tag) + niche_variants.start()
// into
// cast(tag + (niche_variants.start() - niche_start))
// if either the casted type is no larger than the original
// type, or if the niche values are contiguous (in either the
// signed or unsigned sense).
let can_incr = cast_smaller || niches_ule || niches_sle;

let data_for_boundary_niche = || -> Option<(IntCC, u128)> {
if !can_incr {
None
} else if niche_start == low_unsigned {
Some((IntCC::UnsignedLessThanOrEqual, niche_end))
} else if niche_end == high_unsigned {
Some((IntCC::UnsignedGreaterThanOrEqual, niche_start))
} else if niche_start == low_signed {
Some((IntCC::SignedLessThanOrEqual, niche_end))
} else if niche_end == high_signed {
Some((IntCC::SignedGreaterThanOrEqual, niche_start))
} else {
None
}
};

let (is_niche, tagged_discr, delta) = if relative_max == 0 {
// Best case scenario: only one tagged variant. This will
// likely become just a comparison and a jump.
Expand All @@ -206,41 +152,6 @@ pub(crate) fn codegen_get_discriminant<'tcx>(
let tagged_discr =
fx.bcx.ins().iconst(cast_to, niche_variants.start().as_u32() as i64);
(is_niche, tagged_discr, 0)
} else if let Some((predicate, constant)) = data_for_boundary_niche() {
// The niche values are either the lowest or the highest in
// `range`. We can avoid the first subtraction in the
// algorithm.
// The algorithm is now this:
// is_niche = tag <= niche_end
// discr = if is_niche {
// cast(tag + (niche_variants.start() - niche_start))
// } else {
// untagged_variant
// }
// (the first line may instead be tag >= niche_start,
// and may be a signed or unsigned comparison)
// The arithmetic must be done before the cast, so we can
// have the correct wrapping behavior. See issue #104519 for
// the consequences of getting this wrong.
let is_niche = codegen_icmp_imm(fx, predicate, tag, constant as i128);
let delta = (niche_variants.start().as_u32() as u128).wrapping_sub(niche_start);
let incr_tag = if delta == 0 {
tag
} else {
let delta = match fx.bcx.func.dfg.value_type(tag) {
types::I128 => {
let lsb = fx.bcx.ins().iconst(types::I64, delta as u64 as i64);
let msb = fx.bcx.ins().iconst(types::I64, (delta >> 64) as u64 as i64);
fx.bcx.ins().iconcat(lsb, msb)
}
ty => fx.bcx.ins().iconst(ty, delta as i64),
};
fx.bcx.ins().iadd(tag, delta)
};

let cast_tag = clif_intcast(fx, incr_tag, cast_to, !niches_ule);

(is_niche, cast_tag, 0)
} else {
// The special cases don't apply, so we'll have to go with
// the general algorithm.
Expand Down
92 changes: 0 additions & 92 deletions compiler/rustc_codegen_ssa/src/mir/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
) -> V {
let dl = &bx.tcx().data_layout;
let cast_to_layout = bx.cx().layout_of(cast_to);
let cast_to_size = cast_to_layout.layout.size();
let cast_to = bx.cx().immediate_backend_type(cast_to_layout);
if self.layout.abi.is_uninhabited() {
return bx.cx().const_poison(cast_to);
Expand Down Expand Up @@ -261,21 +260,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
_ => (tag_imm, bx.cx().immediate_backend_type(tag_op.layout)),
};

let tag_size = tag_scalar.size(bx.cx());
let max_unsigned = tag_size.unsigned_int_max();
let max_signed = tag_size.signed_int_max() as u128;
let min_signed = max_signed + 1;
let relative_max = niche_variants.end().as_u32() - niche_variants.start().as_u32();
let niche_end = niche_start.wrapping_add(relative_max as u128) & max_unsigned;
let range = tag_scalar.valid_range(bx.cx());

let sle = |lhs: u128, rhs: u128| -> bool {
// Signed and unsigned comparisons give the same results,
// except that in signed comparisons an integer with the
// sign bit set is less than one with the sign bit clear.
// Toggle the sign bit to do a signed comparison.
(lhs ^ min_signed) <= (rhs ^ min_signed)
};

// We have a subrange `niche_start..=niche_end` inside `range`.
// If the value of the tag is inside this subrange, it's a
Expand All @@ -291,49 +276,6 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
// untagged_variant
// }
// However, we will likely be able to emit simpler code.

// Find the least and greatest values in `range`, considered
// both as signed and unsigned.
let (low_unsigned, high_unsigned) = if range.start <= range.end {
(range.start, range.end)
} else {
(0, max_unsigned)
};
let (low_signed, high_signed) = if sle(range.start, range.end) {
(range.start, range.end)
} else {
(min_signed, max_signed)
};

let niches_ule = niche_start <= niche_end;
let niches_sle = sle(niche_start, niche_end);
let cast_smaller = cast_to_size <= tag_size;

// In the algorithm above, we can change
// cast(relative_tag) + niche_variants.start()
// into
// cast(tag + (niche_variants.start() - niche_start))
// if either the casted type is no larger than the original
// type, or if the niche values are contiguous (in either the
// signed or unsigned sense).
let can_incr = cast_smaller || niches_ule || niches_sle;

let data_for_boundary_niche = || -> Option<(IntPredicate, u128)> {
if !can_incr {
None
} else if niche_start == low_unsigned {
Some((IntPredicate::IntULE, niche_end))
} else if niche_end == high_unsigned {
Some((IntPredicate::IntUGE, niche_start))
} else if niche_start == low_signed {
Some((IntPredicate::IntSLE, niche_end))
} else if niche_end == high_signed {
Some((IntPredicate::IntSGE, niche_start))
} else {
None
}
};

let (is_niche, tagged_discr, delta) = if relative_max == 0 {
// Best case scenario: only one tagged variant. This will
// likely become just a comparison and a jump.
Expand All @@ -349,40 +291,6 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
let tagged_discr =
bx.cx().const_uint(cast_to, niche_variants.start().as_u32() as u64);
(is_niche, tagged_discr, 0)
} else if let Some((predicate, constant)) = data_for_boundary_niche() {
// The niche values are either the lowest or the highest in
// `range`. We can avoid the first subtraction in the
// algorithm.
// The algorithm is now this:
// is_niche = tag <= niche_end
// discr = if is_niche {
// cast(tag + (niche_variants.start() - niche_start))
// } else {
// untagged_variant
// }
// (the first line may instead be tag >= niche_start,
// and may be a signed or unsigned comparison)
// The arithmetic must be done before the cast, so we can
// have the correct wrapping behavior. See issue #104519 for
// the consequences of getting this wrong.
let is_niche =
bx.icmp(predicate, tag, bx.cx().const_uint_big(tag_llty, constant));
let delta = (niche_variants.start().as_u32() as u128).wrapping_sub(niche_start);
let incr_tag = if delta == 0 {
tag
} else {
bx.add(tag, bx.cx().const_uint_big(tag_llty, delta))
};

let cast_tag = if cast_smaller {
bx.intcast(incr_tag, cast_to, false)
} else if niches_ule {
bx.zext(incr_tag, cast_to)
} else {
bx.sext(incr_tag, cast_to)
};

(is_niche, cast_tag, 0)
} else {
// The special cases don't apply, so we'll have to go with
// the general algorithm.
Expand Down
2 changes: 1 addition & 1 deletion src/tools/tidy/src/ui_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use std::path::{Path, PathBuf};
// FIXME: The following limits should be reduced eventually.
const ENTRY_LIMIT: usize = 885;
const ROOT_ENTRY_LIMIT: usize = 891;
const ISSUES_ENTRY_LIMIT: usize = 1978;
const ISSUES_ENTRY_LIMIT: usize = 1977;

fn check_entries(tests_path: &Path, bad: &mut bool) {
let mut directories: HashMap<PathBuf, usize> = HashMap::new();
Expand Down
7 changes: 5 additions & 2 deletions tests/codegen/enum-match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,11 @@ pub enum Enum1 {

// CHECK: define noundef i8 @match1{{.*}}
// CHECK-NEXT: start:
// CHECK-NEXT: [[DISCR:%.*]] = {{.*}}call i8 @llvm.usub.sat.i8(i8 %0, i8 1)
// CHECK-NEXT: switch i8 [[DISCR]], label {{.*}} [
// CHECK-NEXT: %1 = add i8 %0, -2
// CHECK-NEXT: %2 = zext i8 %1 to i64
// CHECK-NEXT: %3 = icmp ult i8 %1, 2
// CHECK-NEXT: %4 = add nuw nsw i64 %2, 1
// CHECK-NEXT: %_2 = select i1 %3, i64 %4, i64 0
#[no_mangle]
pub fn match1(e: Enum1) -> u8 {
use Enum1::*;
Expand Down
10 changes: 0 additions & 10 deletions tests/ui/enum-discriminant/issue-104519.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,4 @@ fn some_match(result: OpenResult) -> u8 {
fn main() {
let result = OpenResult::Ok(());
assert_eq!(some_match(result), 0);

let result = OpenResult::Ok(());
match result {
OpenResult::Ok(()) => (),
_ => unreachable!("message a"),
}
match result {
OpenResult::Ok(()) => (),
_ => unreachable!("message b"),
}
}
21 changes: 16 additions & 5 deletions tests/ui/issues/issue-61696.rs → tests/ui/enum-discriminant/issue-61696.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,23 @@ pub enum E2<X> {
V4,
}

fn main() {
if let E1::V2 { .. } = (E1::V1 { f: true }) {
unreachable!()
#[inline(never)]
fn match_e1(y: E1) -> u8 {
match y {
E1::V2 { .. } => 1,
_ => 0,
}
}

if let E2::V1 { .. } = E2::V3::<Infallible> {
unreachable!()
#[inline(never)]
fn match_e2(y: E2<Infallible>) -> u8 {
match y {
E2::V1 { .. } => 1,
_ => 0,
}
}

fn main() {
assert_eq!(match_e1(E1::V1 { f: true }), 0);
assert_eq!(match_e2(E2::V3), 0);
}