Skip to content

Commit 397db05

Browse files
committed
Auto merge of rust-lang#75529 - bugadani:bounds-check, r=nagisa
Eliminate some other bound checks when index comes from an enum rust-lang#36962 introduced an assumption for the upper limit of the enum's value. This PR adds an assumption to the lower value as well. I've modified the original codegen test to show that derived (in that case, adding 1) values also don't generate bounds checks. However, this test is actually carefully crafted to not hit a bug: if the enum's variants are modified to 1 and 2 instead of 2 and 3, the test fails by adding a bounds check. I suppose this is an LLVM issue and rust-lang#75525, while not exactly in this context should be tracking it. I'm not at all confident if this patch can be accepted, or even if it _should_ be accepted in this state. But I'm curious about what others think :) ~Improves~ Should improve rust-lang#13926 but does not close it because it's not exactly predictable, where bounds checks may pop up against the assumptions.
2 parents e88e908 + 1d157ce commit 397db05

File tree

4 files changed

+77
-5
lines changed

4 files changed

+77
-5
lines changed

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

+21-5
Original file line numberDiff line numberDiff line change
@@ -327,13 +327,29 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
327327
if er.end != er.start
328328
&& scalar.valid_range.end() > scalar.valid_range.start()
329329
{
330-
// We want `table[e as usize]` to not
330+
// We want `table[e as usize ± k]` to not
331331
// have bound checks, and this is the most
332-
// convenient place to put the `assume`.
333-
let ll_t_in_const =
332+
// convenient place to put the `assume`s.
333+
if *scalar.valid_range.start() > 0 {
334+
let enum_value_lower_bound = bx
335+
.cx()
336+
.const_uint_big(ll_t_in, *scalar.valid_range.start());
337+
let cmp_start = bx.icmp(
338+
IntPredicate::IntUGE,
339+
llval,
340+
enum_value_lower_bound,
341+
);
342+
bx.assume(cmp_start);
343+
}
344+
345+
let enum_value_upper_bound =
334346
bx.cx().const_uint_big(ll_t_in, *scalar.valid_range.end());
335-
let cmp = bx.icmp(IntPredicate::IntULE, llval, ll_t_in_const);
336-
bx.assume(cmp);
347+
let cmp_end = bx.icmp(
348+
IntPredicate::IntULE,
349+
llval,
350+
enum_value_upper_bound,
351+
);
352+
bx.assume(cmp_end);
337353
}
338354
}
339355
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// This test checks an optimization that is not guaranteed to work. This test case should not block
2+
// a future LLVM update.
3+
// compile-flags: -O
4+
// min-llvm-version: 11.0
5+
6+
#![crate_type = "lib"]
7+
8+
pub enum Bar {
9+
A = 1,
10+
B = 3,
11+
}
12+
13+
// CHECK-LABEL: @lookup_inc
14+
#[no_mangle]
15+
pub fn lookup_inc(buf: &[u8; 5], f: Bar) -> u8 {
16+
// CHECK-NOT: panic_bounds_check
17+
buf[f as usize + 1]
18+
}
19+
20+
// CHECK-LABEL: @lookup_dec
21+
#[no_mangle]
22+
pub fn lookup_dec(buf: &[u8; 5], f: Bar) -> u8 {
23+
// CHECK-NOT: panic_bounds_check
24+
buf[f as usize - 1]
25+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// This test checks an optimization that is not guaranteed to work. This test case should not block
2+
// a future LLVM update.
3+
// compile-flags: -O
4+
// min-llvm-version: 11.0
5+
6+
#![crate_type = "lib"]
7+
8+
#[repr(u8)]
9+
pub enum Exception {
10+
Low = 5,
11+
High = 10,
12+
}
13+
14+
// CHECK-LABEL: @access
15+
#[no_mangle]
16+
pub fn access(array: &[usize; 12], exc: Exception) -> usize {
17+
// CHECK-NOT: panic_bounds_check
18+
array[(exc as u8 - 4) as usize]
19+
}

src/test/codegen/enum-bounds-check.rs

+12
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,15 @@ pub fn lookup(buf: &[u8; 2], f: Foo) -> u8 {
1212
// CHECK-NOT: panic_bounds_check
1313
buf[f as usize]
1414
}
15+
16+
pub enum Bar {
17+
A = 2,
18+
B = 3
19+
}
20+
21+
// CHECK-LABEL: @lookup_unmodified
22+
#[no_mangle]
23+
pub fn lookup_unmodified(buf: &[u8; 5], f: Bar) -> u8 {
24+
// CHECK-NOT: panic_bounds_check
25+
buf[f as usize]
26+
}

0 commit comments

Comments
 (0)