Stream: git-wasmtime

Topic: wasmtime / PR #6708 Fix 5916 interpreter wrong for vall true

view this post on Zulip Wasmtime GitHub notifications bot (Jul 08 2023 at 10:29):

ms140569 opened PR #6708 from ms140569:fix-5916-interpreter-wrong-for-vall_true to bytecodealliance:main:

This fixes: https://github.com/bytecodealliance/wasmtime/issues/5916

view this post on Zulip Wasmtime GitHub notifications bot (Jul 08 2023 at 10:30):

ms140569 requested fitzgen for a review on PR #6708.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 08 2023 at 10:30):

ms140569 requested wasmtime-compiler-reviewers for a review on PR #6708.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 09 2023 at 13:14):

afonso360 submitted PR review:

This looks good, thanks for working on this! Just a few minor corrections.

And also thanks for adding in iter_lanes which is something I've been wanting to add for a while but haven't had time to do.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 09 2023 at 13:14):

afonso360 submitted PR review:

This looks good, thanks for working on this! Just a few minor corrections.

And also thanks for adding in iter_lanes which is something I've been wanting to add for a while but haven't had time to do.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 09 2023 at 13:14):

afonso360 created PR review comment:

Small Nit

    // An iterator over the lanes of a SIMD type

view this post on Zulip Wasmtime GitHub notifications bot (Jul 09 2023 at 13:14):

afonso360 created PR review comment:

I think we don't need the into_int_signed and the from_integer, If I'm reading this correctly we can just clone the value from the simd vec right?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 09 2023 at 13:14):

afonso360 created PR review comment:

Lets propagate up the error from extractlanes. It might help debug something in the future, instead of silently having an empty iterator.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 09 2023 at 13:14):

afonso360 created PR review comment:

I'm not very comfortable with this definition of is_zero for SIMD types. Using any means that we can have non zero values in a lane and this can still return true. And it would still be wrong for Float SIMD types where we can have a zero mantissa or exponent byte while the overall value is still non zero.

I think the best option here would be to return InvalidType for V64 and V128 since we don't really know which SIMD type this DataValue represents.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 09 2023 at 13:14):

afonso360 submitted PR review:

This looks good, thanks for working on this! :tada: Just a few minor corrections.

And also thanks for adding in iter_lanes which is something I've been wanting to add for a while but haven't had time to do.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 12:28):

ms140569 updated PR #6708.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 12:49):

ms140569 updated PR #6708.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 12:53):

ms140569 created PR review comment:

Fixed with:
https://github.com/ms140569/wasmtime/commit/815c6b3e285a9661be28798d61848b0302bf2797

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 12:54):

ms140569 created PR review comment:

Good idea, fixed with:
https://github.com/ms140569/wasmtime/commit/815c6b3e285a9661be28798d61848b0302bf2797

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 12:55):

ms140569 created PR review comment:

Yes, you are right. Fixed:
https://github.com/ms140569/wasmtime/commit/1422505d36b50372ac85f2d23e94548e3e7892ec

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 12:58):

ms140569 created PR review comment:

Yes, you are right. I've used ValueTypeClass::Float for both Vector-Types Error-reply. This was the default anyway before adding all the branches.

Fixed with: https://github.com/ms140569/wasmtime/commit/0bd255231021a49eb944b27aaa66049056b5f39c

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 21:17):

afonso360 submitted PR review:

Small nit but LGTM! :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 21:17):

afonso360 submitted PR review:

Small nit but LGTM! :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 10 2023 at 21:17):

afonso360 created PR review comment:

            DataValue::V64(_) | DataValue::V128(_) => Err(ValueError::InvalidType(ValueTypeClass::Float, self.ty())),

view this post on Zulip Wasmtime GitHub notifications bot (Jul 11 2023 at 07:12):

ms140569 updated PR #6708.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 11 2023 at 07:16):

ms140569 updated PR #6708.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 11 2023 at 07:57):

afonso360 has enabled auto merge for PR #6708.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 11 2023 at 09:57):

ms140569 updated PR #6708.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 11 2023 at 11:11):

afonso360 merged PR #6708.

Last updated: Nov 22 2024 at 16:03 UTC