Correctly implement LLVM's fptoui/fptosi

[dschuff]

WebAssembly's float->int conversion instructions trap if the float value cannot fit in the result type. In C, this conversion overflow is undefined behavior. However, no other architecture has trapping conversions; overflowing conversions generally just yield some arbitrary value. Correspondingly, LLVM's (e.g. https://llvm.org/docs/LangRef.html#fptosi-to-instruction) say that "the result is undefined" in case of overflow. This means the operation is safe to execute speculatively as long as the undefined results are not used. This means that (valid) code of the form

double f; int i;
if([f is in range]) {
 i = (int)f;
} else {
 i = 0;
}

Can be (and is) converted by the optimizer into

%i.conv = fptosi double %f to i32
%inrange = <f's value fits in i32>
%i = select i1 %inrange, i32 %i.conv, i32 0

On wasm, this will trap even thought the original code is correct.

There are a couple of steps to solving it

1. Create target-specific intrinsics @llvm.wasm.fptosi and @llvm.wasm.fptoui(or maybe llvm.wasm.trappingfptosi, which have the same semantics as their corresponding LLVM instructions, except that they trap. Clang will emit these instead of the LLVM instructions, and the optimizers will leave them alone. This will ensure that any conversions guarded as in the example above will not be speculatively executed, and any correct C program (i.e. no UB) will work.
2. There's still the problem of correctly lowering the existing LLVM instructions. Clang will not generate them, but other frontends might. For this case I'd suggest emitting a libcall to a function which bounds-checks the input before conversion. If the performance of that becomes an issue for a particular user, then we can discuss other options specifically.
3. Some code (e.g. BananaBread) goes ahead and executes the overflowing conversion, and then fixes up the result if it's out of range. This is UB but if it works on every other architecture we might want to accommodate that use case. For this we can support BINARYEN_TRAP_MODE with the wasm backend just as we do with asm.js. This is issue Support BINARYEN_TRAP_MODEs for upstream wasm backend WebAssembly/binaryen#1143

[dschuff]

Also BINARYEN_TRAP_MODE isn't really a good name; in general we probably shouldn't use "Binaryen" in flags. Don't know if it's too late to fix that or not.

[kripken]

Yeah, we can rename those (but probably still support the BINARYEN_ name for backwards compatibility). The reason it's BINARYEN_ now is because some of those options were experimental, it wasn't clear those would make sense in the wasm backend too. How about WASM_ prefixes?

For option 3, as discussed earlier this week, we can move the trap code out of asm2wasm into a binaryen pass, then just run that pass on wasm backend output, should be very simple to do. If we do that we should be ok for now, and eventually wasm may get non-trapping math ops anyhow.

[dschuff]

WASM_ probably makes sense as a prefix. And yes I agree with that plan for option 3.

[kripken]

@jgravelle-google did option 3 (move trap code into a binaryen pass) - what's left to do here? do we still need to hook up the wasm backend to use that?

[jgravelle-google]

The s2wasm path currently in emscripten respects BINARYEN_TRAP_MODE settings as of #5620.

This isn't so much a case of "there are three options to support" so much as it is "there are three steps we need to take for full support." So we still need to do 1. and 2. to handle all the cases.

[kripken]

I see, thanks, I misread the above as being separate options.

[jgravelle-google]

Part 2 was implemented by Dan in llvm-mirror/llvm@9f86840

For better performance we still need to implement Part 1, but semantically we're ok now.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.

[aheejin]

Would llvm/llvm-project@232fd99 qualify as the part 1? @tlively

[tlively]

Yes, we now have the platform-specific intrinsics. But currently users have to opt in to them by using a compiler builtin function and clang does not emit them for normal conversions. I expect the real fix here is to turn on the nontrapping-fptoint target feature by default. I'm not sure if there are any engines remaining that do not implement it.

[aheejin]

If the engines are the only problem remaining, would it be safe to close and remove this from the LLVM backend project then?

[aheejin]

Talked with @tlively and @dschuff offline. One more thing that's nice to have is to make clang use the new intrinsics llvm/llvm-project@232fd99 added, so that we don't incur the extra cost in the backend. I'll leave this open for now because there's still a small room for improvement.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

[tlively]

We're still waiting on Safari to ship the nontrapping float-to-int feature. Once that happens, we could consider using those instructions by default in the LLVM backend. In the meantime, I would like to keep this open as a tracking issue. Stale-bot, ping me again in a year!

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

[dschuff]

#15376 tracks updating default features, and we were never planning to do option 1 above, so I think we can close this.