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
ms140569 requested fitzgen for a review on PR #6708.
ms140569 requested wasmtime-compiler-reviewers for a review on PR #6708.
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.
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.
afonso360 created PR review comment:
Small Nit
// An iterator over the lanes of a SIMD type
afonso360 created PR review comment:
I think we don't need the
into_int_signed
and thefrom_integer
, If I'm reading this correctly we can just clone the value from the simd vec right?
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.
afonso360 created PR review comment:
I'm not very comfortable with this definition of
is_zero
for SIMD types. Usingany
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
forV64
andV128
since we don't really know which SIMD type thisDataValue
represents.
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.
ms140569 updated PR #6708.
ms140569 updated PR #6708.
ms140569 created PR review comment:
Fixed with:
https://github.com/ms140569/wasmtime/commit/815c6b3e285a9661be28798d61848b0302bf2797
ms140569 created PR review comment:
Good idea, fixed with:
https://github.com/ms140569/wasmtime/commit/815c6b3e285a9661be28798d61848b0302bf2797
ms140569 created PR review comment:
Yes, you are right. Fixed:
https://github.com/ms140569/wasmtime/commit/1422505d36b50372ac85f2d23e94548e3e7892ec
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
afonso360 submitted PR review:
Small nit but LGTM! :+1:
afonso360 submitted PR review:
Small nit but LGTM! :+1:
afonso360 created PR review comment:
DataValue::V64(_) | DataValue::V128(_) => Err(ValueError::InvalidType(ValueTypeClass::Float, self.ty())),
ms140569 updated PR #6708.
ms140569 updated PR #6708.
afonso360 has enabled auto merge for PR #6708.
ms140569 updated PR #6708.
afonso360 merged PR #6708.
Last updated: Dec 23 2024 at 13:07 UTC