Stream: git-wasmtime

Topic: wasmtime / issue #4446 cranelift: Add `fadd`/`fsub`/`fmul...

Wasmtime GitHub notifications bot (Jul 14 2022 at 17:31):

afonso360 commented on issue #4446:

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.

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.

Wasmtime GitHub notifications bot (Jul 14 2022 at 17:47):

afonso360 edited a comment on issue #4446:

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.

Wasmtime GitHub notifications bot (Jul 14 2022 at 17:51):

jameysharp commented on issue #4446:

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!

Wasmtime GitHub notifications bot (Jul 14 2022 at 18:13):

afonso360 commented on issue #4446:

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

Wasmtime GitHub notifications bot (Jul 14 2022 at 18:21):

jameysharp commented on issue #4446:

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

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

Wasmtime GitHub notifications bot (Jul 14 2022 at 21:19):

afonso360 commented on issue #4446:

Oops, should be fixed now.

Last updated: Dec 23 2024 at 12:05 UTC