cranelift: Add fadd/fsub/fmul/fdiv to interpreter

[afonso360]

👋 Hey,

Fuzzgen found that we were missing these as soon as I added float support

[jameysharp]

I'm still learning the details of the WebAssembly spec so I have a couple questions, but overall I think this looks good.

I expected to see fdiv tests for the case of a non-zero value divided by zero, which I assumed would return NaN, but I didn't see such tests in that section. Did I miss something?

In the implementation of the interpreter, would it make sense to have all integer values be represented using std::num::Wrapping? Then I think all the uses of binary_match! that you updated here could just use e.g. +(self, other) for both integer and floating point types, and you wouldn't need an is_float function.

[afonso360]

I expected to see fdiv tests for the case of a non-zero value divided by zero, which I assumed would return NaN, but I didn't see such tests in that section. Did I miss something?

You're absolutely right, we are missing those cases. Ill add them. However those cases return +/-Inf and not NaN.

Looking at the WASM spec for fdiv I think there might be some other corner cases that I also missed in these tests.

In the implementation of the interpreter, would it make sense to have all integer values be represented using std::num::Wrapping? Then I think all the uses of binary_match! that you updated here could just use e.g. +(self, other) for both integer and floating point types, and you wouldn't need an is_float function.

That sounds like a good idea for the interpreter. However DataValue is shared with the rest of the compiler and I think that will have some knock on effects which I'm not quite sure of the implications that would bring.

I'm willing to try and make that change if you think its worth it.

[jameysharp]

I'm interested in finding out if using Wrapping everywhere would clean things up, but let's merge this PR without waiting for that investigation. I hoped it might be a quick clean-up, but it sounds like it isn't, and it's not important.

Let me know when you're done with the fdiv tests and I'm happy to merge this!

[afonso360]

I think we have them all now. Thanks for reviewing this!

[jameysharp]

You're welcome! Code review is good for me, I learn things. 😆

Did you mean to comment out test interpret in that last force-push, or was that an accident?

[afonso360]

Oops, should be fixed now.