LLVM wasm backend emitting non-trapping fp-to-int conversion sequences

[sunfishcode]

WIth LLVM r319128, LLVM is now translating fptosi/fptoui into sequences which don't trap when the input is outside their domain. This fixes the associated LLVM bugs. It also means that Binaryen won't be able to provide configurable fp-to-int trapping behavior when used with LLVM wasm backend output.

This feature will be precluded by the nontrapping float-to-int conversion feature being added to wasm eventually anyway, so unless there are reasons to keep it, it seems better to me to omit this feature from the LLVM wasm backend rather than do something more complex to support it temporarily.

The test test_binaryen_trap_mode is currently failing with the LLVM wasm backend; assuming the various modes won't be supported, this test should be disabled.

[kripken]

precluded

Typo? Or do I not understand that word? :)

This feature will be precluded by the nontrapping float-to-int conversion feature being added to wasm eventually anyway

Won't the trapping ones be smaller and faster?

[sunfishcode]

This feature will be precluded by the nontrapping float-to-int conversion feature

I meant "This feature" to refer to Binaryen's trap-mode feature, which is the thing which would be precluded when LLVM eventually switches to the non-trapping instructions.

Won't the trapping ones be smaller and faster?

They're one byte smaller in wasm, but these operations aren't frequent enough to be significant.

On ARM, the non-trapping instruction will map to a single instruction, which will be faster than the trapping version. On x86, trapping and non-trapping both need extra checks, so they're not significantly different in speed.

[kripken]

I wasn't just kidding before: Do you literally mean "preclude" as in "make impossible"?

I think we will still want a trap mode, but it might have 2 options rather than 3: "normal" (the non-trapping mode wasm is adding) and "js". The latter is useful for debugging. That's why I'm asking if this change would make that impossible. Or perhaps we should consider removing it too?

[sunfishcode]

I meant that it would impossible for Binaryen to implement trapping behavior reliably, because LLVM may have speculated the conversion past a branch.

However, are you thinking of having Binaryen have a mode to insert traps for non-trapping instructions anyway? I agree that that's doable, with the caveat that it'll sometimes have some false positives. In fact, I added support for non-trapping instructions already -- enable them in clang with -fnontrapping-fptoint -mnontrapping-fptoint or llc with -mattr=+nontrapping-fptoint -- so you could even implement this today without having to pattern-match the code LLVM is now emitting.

[kripken]

Sorry, maybe I wasn't clear. I'm talking about the exact opposite: the second mode would be "js" which does not trap, it applies JS semantics. For existing asm.js users, it helps debug things as it removes a source of wasm/asm.js divergence.

It seems like that could be done by just replacing the operations with the JS versions? Or am I missing something?

[sunfishcode]

Sure; you could do that too. Enable the non-trapping operators in LLVM with the flags I mentioned above, and then have Binaryen replace those operators with whatever you want.

[kripken]

Sounds good.

So, how are we moving forward here? In the short term, this creates a difference between asm2wasm and the wasm backend output, which makes migration to the wasm backend by default less smooth. We can disable that test, but the difference is still there. Should we move to change asm2wasm in a similar way? Or something else?

[sunfishcode]

I suggest disabling the test for now. My assumption is that the specific behavior, other than not trapping, is not very significant for C/C++ code. In all the discussions of float-to-int conversion operators, I myself haven't heard from real-world use cases that care what the behavior is, except for cases that just need it to be non-trapping. It's UB in C/C++. It's flagged as a bug by UBSan. x86, ARM, Power, RISC-V, and JS all have different behavior on float-to-int conversion, and I'm not aware of other C++ compilers offering features to emulate the behavior of different platforms.

[kripken]

I agree real-world code generally doesn't care. But a use case for us devs is debugging a program that works in asm2wasm but fails in the wasm backend or vice versa: if they don't have identical behavior on this stuff, execution logs diverge, when they might otherwise be easily diffed and the bug found.

[jgravelle-google]

I think the level of sophistication of how we should handle this is dictated by the question "will we want the js trap mode in five years?" I'm thinking no, because by that point we should only have the wasm-via-LLVM path, so anything we do here should be considered transitional work that gets us there without too much breakage.

For the immediate future, that means disabling the test.
After that, I think the best we can do is remove binaryen trap mode from s2wasm, and make the clamp mode in asm2wasm match what the backend emits. Is that reasonable?

[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.