Redundant checks in floating point to integer casts on WASM

[CryZe]

If you compile the following Rust code to WASM:

pub unsafe fn cast(x: f64) -> u8 {
    x.to_int_unchecked()
}

it compiles to the following with Rust 1.45:

example::cast:
        block           
        local.get       0
        f64.const       0x1p32
        f64.lt  
        local.get       0
        f64.const       0x0p0
        f64.ge  
        i32.and 
        i32.eqz
        br_if           0
        local.get       0
        i32.trunc_f64_u
        return
        end_block
        i32.const       0
        end_function

The same happens in Rust <1.45 if you use as _. In both cases the following LLVM IR is emitted:

define hidden i8 @_ZN7example4cast17he66904755097d668E(double %x) unnamed_addr #0 !dbg !5 {
  %0 = fptoui double %x to i8, !dbg !9
  ret i8 %0, !dbg !22
}

fptoui is explicitly defined to be UB if an out of range value is provided.

If the value cannot fit in ty2, the result is a poison value.

However it seems like the WASM backend in LLVM ignores that and emits additional bounds checks.

Additionally this gets even worse in Rust 1.45 if you use as _ instead:

example::cast:
        local.get       0
        f64.const       0x1.fep7
        f64.gt  
        local.set       1
        block           
        block           
        local.get       0
        f64.const       0x0p0
        local.get       0
        f64.const       0x0p0
        f64.gt  
        f64.select
        local.tee       0
        f64.const       0x1p32
        f64.lt  
        local.get       0
        f64.const       0x0p0
        f64.ge  
        i32.and 
        i32.eqz
        br_if           0
        local.get       0
        i32.trunc_f64_u
        local.set       2
        br              1
        end_block
        i32.const       0
        local.set       2
        end_block
        i32.const       -1
        local.get       2
        local.get       1
        i32.select
        end_function

This means that in the end in the WASM VM will do 3 range checks (the one emitted by rust, the one emitted by the llvm backend, and the one it needs to do to possibly trap).

Additionally Rust's saturating checks don't play well with WASM's nontrapping-fptoint target-feature as there's still redundant checks:

example::cast:
        i64.const       9223372036854775807
        local.get       0
        f32.const       -0x1p63
        local.get       0
        f32.const       -0x1p63
        f32.gt  
        f32.select
        i64.trunc_sat_f32_s
        local.get       0
        f32.const       0x1.fffffep62
        f32.gt  
        i64.select
        i64.const       0
        local.get       0
        local.get       0
        f32.eq  
        i64.select
        end_function

[CryZe]

It seems to come down to this commit by @sunfishcode in LLVM llvm/llvm-project@cdd48b8

I fail to understand the justification here:

// Lower an fp-to-int conversion operator from the LLVM opcode, which has an
// undefined result on invalid/overflow, to the WebAssembly opcode, which
// traps on invalid/overflow.

If the LLVM opcode doesn't define the behavior for invalid results, then why is the WASM backend trying to paper over the UB and instead introduces checks to supposedly trap? (which I don't think it even does, it just returns dummy values)

[sunfishcode]

In the context of that comment, "undefined result" doesn't mean "undefined behavior". It means the operation produces a return value, without trapping, but the specific value is undefined. Since's the wasm operator in question traps in some questions, extras checks are required to prevent trapping, to emulate the LLVM IR semantics.

#71269 has now landed, so Rust's semantics for fp-to-int conversion match those of wasm's nontrapping-fptoint feature, which is now part of wasm's core spec, though not all engines implement it yet. So for anyone interested in working on this, the path forward is to teach the Rust backend to take advantage of this wasm feature, when present, to avoid emitting the extra range checks.

[CryZe]

So from what I'm understanding from more code, LLVM could be reordering fptoui / fptosi to be speculatively executed earlier, which would mean the code traps when it possibly shouldn't have, as that cast may have never been executed. Is that correct?

There also seem to be LLVM intrinsics in the WASM backend now such as i32 @llvm.wasm.trunc.signed.i32.f32(float). Can those be speculatively executed? If no, the code should probably be lowered like this:

	-nontrapping-fptoint	+nontrapping-fptoint
.to_int_unchecked()	llvm.wasm.trunc.*	llvm.wasm.trunc.*
as _	llvm.wasm.trunc.* + manual saturation checks	llvm.wasm.trunc.saturate.*

If yes, it should probably be this:

	-nontrapping-fptoint	+nontrapping-fptoint
.to_int_unchecked()	fptosi / fptoui	llvm.wasm.trunc.saturate.*
as _	llvm.wasm.trunc.* + manual saturation checks (or maybe that intrinsic is still dangerous in that case?)	llvm.wasm.trunc.saturate.*

[sunfishcode]

Yes, LLVM can reorder and speculate fptosi/fptoui as it assumes they don't trap.

@llvm.wasm.trunc.signed.i32.f32 can trap, so it cannot be speculatively executed in general.

[alexcrichton]

I've posted #74659 to improve the unchecked -nontrapping-fptoint case

[alexcrichton]

Ok, I've improved the last case at #74695 and I think that's good enough to close this out.