Passing enum by-value loses range metadata

[huonw]

Test case update for Rust 1.0:

pub enum Foo {
    A, B, C, D
}

static X: [isize; 4] = [10, 1, 4, 3];

pub fn foo(y: Foo) -> isize {
    X[y as usize]
}

Original report below:

---

pub enum Foo {
    A, B, C, D
}

static X: [int, .. 4] = [10, 1, 4, 3];
pub fn foo(y: Foo) -> int {
    X[y as uint]
}

becomes

; Function Attrs: uwtable
define i64 @_ZN3foo20h19859df9e3d3b0bcsaa4v0.0E(i8) unnamed_addr #0 {
entry-block:
  %1 = zext i8 %0 to i64
  %2 = icmp ugt i8 %0, 3
  br i1 %2, label %cond, label %next, !prof !0

next:                                             ; preds = %entry-block
  %3 = getelementptr inbounds [4 x i64]* @_ZN1X20h61a81bb0de98e867iaa4v0.0E, i64 0, i64 %1
  %4 = load i64* %3, align 8
  ret i64 %4

cond:                                             ; preds = %entry-block
  tail call void @_ZN2rt6unwind17fail_bounds_check20h0f41a1608cc59c5e1199v0.11.preE(i8* getelementptr inbounds ([14 x i8]* @str879, i64 0, i64 0), i64 7, i64 %1, i64 4)
  unreachable
}

even though it's impossible for a Foo to be larger than 3, so the bounds checking can never fail. (Doing a similar thing with e.g. [int, .. 256] and a value of type u8 does eliminate the bounds checks.)

(I guess this is an LLVM bug, but filing it here just in case we're emiting range info incorrectly, or some-such.)

[huonw]

Oh, actually: this is because there is no range metadata when Foo is passed by value: passing it by reference fn foo(x: &Foo) means a load with metadata is emitted and the bounds check is eliminated:

define i64 @_ZN3foo20h043091a9d9208cb0saa4v0.0E(i8* nocapture readonly) unnamed_addr #0 {
entry-block:
  %1 = load i8* %0, align 1, !range !0
  %2 = zext i8 %1 to i64
  %3 = getelementptr inbounds [4 x i64]* @_ZN1X20hcca41332b11ffec7iaa4v0.0E, i64 0, i64 %2
  %4 = load i64* %3, align 8
  ret i64 %4
}

!0 = metadata !{i8 0, i8 4}

Updated title from "indexing fixed-length array doesn't eliminated bounds checks".

[thestinger]

Fixing this would likely require extending the feature in LLVM to be usable on parameters.

[ranma42]

I agree that the !range metadata annotation would be convenient also on values other than those coming from load/call/invoke, but wouldn't it be sufficient to use the llvm.assume intrinsic to specify the constraints on values (in this case, its range)?

[ranma42]

If the enumeration derives Copy and Clone, this seems to be a way to code the function so that the LLVM optimisations generate the desired code:

pub fn foo(y: Foo) -> isize {
    let closure = |z: &Foo| { X[(*z) as usize] };
    closure(&y)
}

; Function Attrs: nounwind readnone uwtable
define i64 @_ZN3foo20h8ac558f51be88dd53aaE(i8) unnamed_addr #1 {
entry-block:
  %1 = zext i8 %0 to i64
  %2 = getelementptr inbounds [4 x i64], [4 x i64]* @_ZN1X20h5e497aaeb62c0700UaaE, i64 0, i64 %1
  %3 = load i64, i64* %2, align 8, !noalias !1
  ret i64 %3
}

This might be fragile as it probably relies on the order in which the LLVM optimiser performs the bound-check optimisation and the closure inlining.

[brson]

Can this be reproed today?

[arielb1]

Sure. Same LLVM problem. I fixed the y as uint case in #36962. The underlying issue still exists.

[bstrie]

Here's the generated LLVM IR as of Rust 1.41:

@_ZN10playground1X17h7a5538bb38d6a168E = internal unnamed_addr constant <{ [32 x i8] }> <{ [32 x i8] c"\0A\00\00\00\00\00\00\00\01\00\00\00\00\00\00\00\04\00\00\00\00\00\00\00\03\00\00\00\00\00\00\00" }>, align 8

; playground::foo
; Function Attrs: nounwind nonlazybind uwtable
define i64 @_ZN10playground3foo17hc0370d8e3b68fd0eE(i8 %y) unnamed_addr #0 {
start:
  %0 = icmp ult i8 %y, 4
  tail call void @llvm.assume(i1 %0)
  %_3 = zext i8 %y to i64
  %1 = getelementptr inbounds [4 x i64], [4 x i64]* bitcast (<{ [32 x i8] }>* @_ZN10playground1X17h7a5538bb38d6a168E to [4 x i64]*), i64 0, i64 %_3
  %2 = load i64, i64* %1, align 8
  ret i64 %2
}

; Function Attrs: nounwind
declare void @llvm.assume(i1) #1

attributes #0 = { nounwind nonlazybind uwtable "probe-stack"="__rust_probestack" "target-cpu"="x86-64" }
attributes #1 = { nounwind }

!llvm.module.flags = !{!0, !1}

!0 = !{i32 7, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}

I'm no expert, but I note that the branch is in fact now gone, however we're still doing a comparison against the input parameter and passing the result of the comparison to llvm.assume; I have no idea what that does, but the fact that the function is also marked nounwind would seem to confirm that rustc knows that this can't ever fail.

[jonas-schievink]

we're still doing a comparison against the input parameter and passing the result of the comparison to llvm.assume

That will never hit codegen, it's just an optimization hint for LLVM.

This issue does still reproduce with this code though:

#[repr(u8)]
pub enum Exception {
    Low = 5,
    High = 10,
}

pub fn access(array: &[usize; 12], exc: Exception) -> usize {
    array[(exc as u8 - 4) as usize]
}

LLVM IR:

define i64 @_ZN10playground6access17h378ae4778586ad8dE([12 x i64]* noalias nocapture readonly align 8 dereferenceable(96) %array, i8 %exc) unnamed_addr #0 {
start:
  %0 = icmp ult i8 %exc, 11
  tail call void @llvm.assume(i1 %0)
  %_4 = add nsw i8 %exc, -4
  %_3 = zext i8 %_4 to i64
  %_8 = icmp ult i8 %_4, 12
  br i1 %_8, label %bb1, label %panic, !prof !2

bb1:                                              ; preds = %start
  %1 = getelementptr inbounds [12 x i64], [12 x i64]* %array, i64 0, i64 %_3
  %2 = load i64, i64* %1, align 8
  ret i64 %2

panic:                                            ; preds = %start
; call core::panicking::panic_bounds_check
  tail call void @_ZN4core9panicking18panic_bounds_check17h09b793daa6d169ffE(%"core::panic::Location"* noalias readonly align 8 dereferenceable(24) bitcast (<{ i8*, [16 x i8] }>* @anon.2bcef93dabe22e774fed7ccaab37f6cf.1 to %"core::panic::Location"*), i64 %_3, i64 12)
  unreachable
}

(this originally showed up in rust-embedded/cortex-m#202)

[bugadani]

#36962 actually fixed the table[enum as usize + x] cases, but adding the assumption that value >= valid_range.start() should (at least in theory) also fix the table[enum as usize - x] cases.

I've attempted to add the assumption but it failed to achieve the desired results.

I've opened #75525 which I believe is related. See this example: https://godbolt.org/z/YhK1rT When any of the assumptions is an inclusive relation check instead of an exclusive one, the assumption is useless.

[erikdesjardins]

@jonas-schievink's test case was added in #75529: see https://github.com/rust-lang/rust/pull/75529/files#diff-d8b41e5a819724c9afcfc0f1ab152c25

This can probably be closed, unless anyone has another example that doesn't get optimized.

[AngelicosPhosphoros]

This now compiles without bounds check.
https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=86371bd06d8bacff494b76c94486ba5b

@huonw Maybe you should close the issue.