Skip to content

Commit

Permalink
Auto merge of #110197 - cjgillot:codegen-discr, r=pnkfelix
Browse files Browse the repository at this point in the history 
Do not attempt to commute comparison and cast to codegen discriminants

The general algorithm to compute a discriminant is:
```
relative_tag = tag - niche_start
is_niche = relative_tag <= (ule) relative_max
discr = if is_niche {
    cast(relative_tag) + niche_variants.start()
} else {
    untagged_variant
}
```

We have an optimization branch which attempts to merge the addition and the subtraction by commuting them with the cast. We currently get this optimization wrong.

This PR takes the easiest and safest way: remove the optimization, and let LLVM handle it. (Perf may not agree with that course of action 😅)

There may be a less invasive solution, but I don't have the necessary knowledge of LLVM semantics to find it. Cranelift has the same optimization, which should be handled similarly.
cc `@nikic` and `@bjorn3` if you have a better solution.

Fixes #110128
  • Loading branch information
@bors
bors committed Apr 14, 2023
2 parents 84dd17b + a8857d2 commit 158c309
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 199 deletions.
89 changes: 0 additions & 89 deletions compiler/rustc_codegen_cranelift/src/discriminant.rs
View file
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
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
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
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
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
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);
}

0 comments on commit 158c309

Please sign in to comment.