jlb6740 opened PR #2291 from packed-convert
to main
:
f32x4.convert_i32x4_s
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jlb6740 updated PR #2291 from packed-convert
to main
:
f32x4.convert_i32x4_s
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jlb6740 updated PR #2291 from packed-convert
to main
:
f32x4.convert_i32x4_s
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jlb6740 submitted PR Review.
jlb6740 created PR Review Comment:
These white spaces are curious. I do not see them indicated on the file I push?
jlb6740 edited PR Review Comment.
jlb6740 updated PR #2291 from packed-convert
to main
:
f32x4.convert_i32x4_s
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jlb6740 updated PR #2291 from packed-convert
to main
:
f32x4.convert_i32x4_s
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jlb6740 updated PR #2291 from packed-convert
to main
:
f32x4.convert_i32x4_s
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jlb6740 updated PR #2291 from packed-convert
to main
:
f32x4.convert_i32x4_s
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jlb6740 requested bnjbvr for a review on PR #2291.
jlb6740 requested cfallin for a review on PR #2291.
jlb6740 requested cfallin, abrown and bnjbvr for a review on PR #2291.
jlb6740 requested cfallin, abrown and bnjbvr for a review on PR #2291.
abrown submitted PR Review.
abrown submitted PR Review.
abrown created PR Review Comment:
_ => unimplemented!("unable to use type {} for op {}", ctx.input_ty(insn, 0), op),
My reasoning: more information is usually better with these errors and fcvt_from_sint can accept more than the
I32X4
type (so other types are unimplemented, not unreachable).
abrown created PR Review Comment:
It's fine:
rustfmt
probably bumped them in a bit since they are now nested insideif !output_ty.is_vector()
.
jlb6740 submitted PR Review.
jlb6740 created PR Review Comment:
@abrown Yes .. agree to more information. The unreachable vs unimplemented I um .. kind of have a different take. Right now, until cliff instructions are added, it is unreachable (I think?). There is no clif instruction (I think?) that can get here .. as this code is in a branch that has already checked for a vector type. Taken in context, the unreachable doesn't refer to just fcvt_from_sint it refers to vector types of fcvt_from_sint in which case there is only one afaik? I will change it though because taken out of context it is reachable and in any case I think with this kind of ambiguity, for convenience power goes to the who reviews :-) .. and of course precedence/consistency. :+1:
jlb6740 updated PR #2291 from packed-convert
to main
:
f32x4.convert_i32x4_s
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
jlb6740 merged PR #2291.
abrown created PR Review Comment:
it refers to vector types of fcvt_from_sint in which case there is only one afaik
Take a look at the types of the fcvt_from_sint instruction I linked to:
x
isInt
, which makes that CLIF instruction polymorphic over all integer and lane sizes. So someone could theoretically write a CLIF instruction with a type that would trigger the error, even if the Wasm translation would never create it.
abrown submitted PR Review.
Last updated: Dec 23 2024 at 13:07 UTC