Rewrite Min/Max to handle all cases correctly. Fixes 545 spectest failures.

[nlewycky]

Description

The llvm backend was not quite following the Wasm spec for {F32,F64,F32x4xF64x2}{Min,Max}. We used the @llvm.minnum and @llvm.maxnum intrinsics which don't handle the corner cases the same. When we tried to use @llvm.minimum and @llvm.maximum which do, we get an internal error from the x86 backend. I was hoping that crash would go away with the upgrade to LLVM 9, but it does not.

Reimplement these operations using plain LLVM instructions.

Review

• Add a short description of the the change to the CHANGELOG.md file

[syrusakbary]

Can you add comments in each of this implementations adding context, such as:

LLVM minmax doesn't work on the versions I tried (8, 9) because ...
So we are writing our own implementation of minmax each time we call `f32min` ...

[nlewycky]

Done.

[syrusakbary]

Apart from the comment added. Rather than adding the IR for minmax each time that we call min on a value, can we just create 4 different functions (in the LLVM IR) and call them?
That way LLVM will decide if it wants to inline them or not (and perhaps the code will become a bit simpler (specially once LLVM fixes certain things).

[nlewycky]

Apart from the comment added. Rather than adding the IR for minmax each time that we call min on a value, can we just create 4 different functions (in the LLVM IR) and call them?
That way LLVM will decide if it wants to inline them or not (and perhaps the code will become a bit simpler (specially once LLVM fixes certain things).

I don't think we should do that? The first reason is that when we call builder.create_some_instruction, it doesn't necessarily create an instruction! If the llvm Value is known, say because it's a constant because the webassembly code happened to have (f32.const 0.0) then the builder will simply return another constant if it can. The code looks like it handles the fully generic case, but it will get specialized down as we write it out.

Secondly, we're in the middle of emitting a function. LLVM is fine with seeing more functions at any time, but LLVMModuleCodeGenerator is not. It expects next_function() to produce a single function which it will then produce machine code for in finalize(). We can fix this of course, but it's not a change that will be confined to the implementation of Operator::F32Min and friends.

[nlewycky]

bors r+

[bors]

Build succeeded

• wasmerio.wasmer