Wasm traps on saturating float casts

[hanna-kruppe]

In https://internals.rust-lang.org/t/help-us-benchmark-saturating-float-casts/6231/19 @alexcrichton reported that wasm traps on out-of-range floats being cast to integers. This is a problem for the current implementation of saturating float casts, which executes the cast unconditionally and then only uses it if the input was in range. LLVM says that the result is undefined (i.e., undef) rather than the behavior being undefined. On all other targets I'm aware of, the operation just gets lowered to an instruction that returns some integer result for those cases.

I've checked the wasm spec, which says:

[truncating a float to an integer] is not defined for NaNs, infinities, or values for which the result is out of range.

As wasm doesn't want everything-goes UB as in C or LLVM or Rust, "undefined" operations are specified to trap. So this is intentional in wasm, which makes sense to me. They'd rather trap than return a questionable result, and they certainly can't tolerate hardware-dependent behavior.

So how do we work around this? We could emit branches instead of unconditionally executing the cast, at the cost of a more complicated implementation (need to juggle multiple basic blocks) and other downsides (branch predictor pressure, input-data-dependent performance). It might also be an argument in favor of making the saturation explicit in MIR rather than doing it during LLVM IR codegen:

• Would avoid introducing multiple basic blocks for one cast rvalue in MIR (could be simpler)
• Would avoid duplicating work if e.g. Cranelift copies the wasm behavior and we eventually get a Cranelift backend.

cc @sunfishcode @eddyb

[sunfishcode]

I haven't look at the rustc side of this in detail yet, but the LLVM wasm backend does have a known bug in this area: LLVM's fptosi/fptoui are documented to (silently) return undef on invalid/overflow, which means that trapping is a bug from LLVM's perspective.

https://reviews.llvm.org/D38524 is a patch which fixes the bug, though it needs a little more work before landing.

Beyond that, there's also the proposal to add non-trapping float-to-int conversion operators to WebAssembly, which is mostly through the standards process and is now in the "implementation phase". Once this is widely implemented, https://reviews.llvm.org/D38524 will become unnecessary. But also, the proposed semantics are the same as Rust's new saturating conversion semantics, so eventually we'll be able to map a Rust float-to-int conversion directly to a single wasm opcode.

[alexcrichton]

Oh awesome thanks for the report @rkruppe and the info @sunfishcode! I'm glad I was misinterpreting the LLVM reference!

Sounds like we should probably just wait for this to get fixed upstream? In the meantime I think it's probably safe to ignore the one test on wasm for now.

[sunfishcode]

I've now landed the LLVM patch upstream (r319128).

[alexcrichton]

Awesome thanks @sunfishcode! It looks like though we probably won't be able to turn that on by default for some time now, right?

If so @rkruppe just to confirm, it'd be a perf loss to change the behavior of saturating casts, right?

[sunfishcode]

@alexcrichton It also changes the default behavior. If you can use LLVM trunk, or backport the patch, I expect it'll fix the test failure.

[alexcrichton]

Oh I missed that, perfect! I'll backport to our fork and get this in master

[alexcrichton]

Ok I've done a backport but it unfortunately didn't fix the test, I was still seeing "integer result unrespresentable".

I minimized it down to:

#[no_mangle]
pub extern fn foo(a: f64) -> bool {
    a as u64 != 0
}

which when invoked as instance.exports.foo(-1) will cause the wasm exception (when rustc compiles with -Z saturating-float-casts. The IR looks like:

define zeroext i1 @foo(double %a) unnamed_addr #0 {
start:
  %0 = fptoui double %a to i64
  %1 = fcmp ogt double %a, 0x43EFFFFFFFFFFFFF
  %2 = icmp ne i64 %0, 0
  %not. = fcmp oge double %a, 0.000000e+00
  %3 = and i1 %not., %2
  %4 = or i1 %1, %3
  ret i1 %4
}

so like before the fptoui instruction is getting executed regardless of the input (and I think causing the trap). The WebAssembly output is https://gist.github.com/alexcrichton/63b514fd49fda6b9e029d28d630073d1 from this.

If I remove the -Z saturating-float-casts then the IR is simply:

define zeroext i1 @foo(double %a) unnamed_addr #0 {
start:
  %0 = fptoui double %a to i64
  %1 = icmp ne i64 %0, 0
  ret i1 %1
}

(as one might expect) and the wasm output is:

 (func $0 (; 0 ;) (type $0) (param $var$0 f64) (result i32)
  (local $var$1 i64)
  (block $label$1
   (block $label$2
    (br_if $label$2
     (i32.eqz
      (f64.lt
       (get_local $var$0)
       (f64.const 18446744073709551615)
      )
     )
    )
    (set_local $var$1
     (i64.trunc_u/f64
      (get_local $var$0)
     )
    )
    (br $label$1)
   )
   (set_local $var$1
    (i64.const 0)
   )
  )
  (i64.ne
   (get_local $var$1)
   (i64.const 0)
  )
)

@sunfishcode did I perhaps mess up the backport? Or is there maybe a missing comparison with 0 for the unsigned types?

[sunfishcode]

Oh, it's missing the comparison with 0 for unsigned types. I'll need to submit another patch.

[sunfishcode]

The missing comparison with 0 is fixed in r319354.

[alexcrichton]

Thanks! I'm testing with that now to ensure we're all good.

[alexcrichton]

It appears to work like a charm, thanks again @sunfishcode and @rkruppe!