Wasmtime GitHub notifications bot (Mar 20 2023 at 17:15): Wasmtime GitHub notifications bot (Mar 20 2023 at 17:15):elliottt opened PR #6070 from trevor/restrict-isplit-iconcat to main:
All backends currently assume that
isplitandiconcatonly work for splittingi128values, or concatenating twoi64values to make ani128value. This PR changes the instruction definition to reflect that assumption.
<!--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.
-->
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:16):elliottt has marked PR #6070 as ready for review.
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:23):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:24):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:24):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:39):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:40):elliottt edited PR #6070 from trevor/restrict-isplit-iconcat to main:
All backends currently assume that
isplitandiconcatonly work for splittingi128values, or concatenating twoi64values to make ani128value. This PR changes the instruction definition to reflect that assumption.This PR makes the
HalfWidthandDoubleWidthconstraints never constructed, but I think we should leave these two constructors marked#[allow(unused)]rather than remove them, as we might want to relax the constraints onisplitandiconcatin the future.
<!--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.
-->
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:46):elliottt edited PR #6070 from trevor/restrict-isplit-iconcat to main:
All backends currently assume that
isplitandiconcatonly work for splittingi128values, or concatenating twoi64values to make ani128value. This PR changes the instruction definition to reflect that assumption.This PR makes the
HalfWidthandDoubleWidthconstraints never constructed as the uses of those constraints in the instruction DSL now produces singleton sets. However, I think we should leave these two constructors marked#[allow(unused)]rather than remove them, as we might want to relax the constraints onisplitandiconcatin the future.
<!--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.
-->
Wasmtime GitHub notifications bot (Mar 20 2023 at 17:59):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 21:16):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 20 2023 at 21:18):elliottt edited PR #6070 from trevor/restrict-isplit-iconcat to main:
All backends currently assume that
isplitandiconcatonly work for splittingi128values, or concatenating twoi64values to make ani128value. The type constraints for bot indicate that they can work over vectors as well as other bit-width integers. This PR resolves part of this discrepancy by removing vector support from bothisplitandiconcat.
<!--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.
-->
Wasmtime GitHub notifications bot (Mar 20 2023 at 22:34):elliottt edited PR #6070 from trevor/restrict-isplit-iconcat to main:
All backends currently assume that
isplitandiconcatonly work for splittingi128values, or concatenating twoi64values to make ani128value. The type constraints for both indicate that they can work over vectors as well as other bit-width integers. This PR resolves part of this discrepancy by removing vector support from bothisplitandiconcat.
<!--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.
-->
Wasmtime GitHub notifications bot (Mar 20 2023 at 23:56):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Mar 20 2023 at 23:56):jameysharp created PR review comment:
The other place that
NarrowIntis used is foriconst. I recently asked Chris about whether we should be making use oficonst's documented ability to construct a vector, and he was surprised that it was documented that way. Perhaps we should just removesimd_lanesfrom the original definition ofNarrowIntinstead of shadowing it here?
Wasmtime GitHub notifications bot (Mar 21 2023 at 00:13):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2023 at 00:13):elliottt created PR review comment:
Happy to make that change as well.
Wasmtime GitHub notifications bot (Mar 21 2023 at 00:14):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 21 2023 at 00:15):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 21 2023 at 00:36):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2023 at 00:36):jameysharp created PR review comment:
Apparently a couple of parser filetests use
iconstwith vector types, but since they're only testing the parser I don't think we should take that as a sign that this is actually used.In
rewrite.clifI think you can just change it to aniconst.i32. Or possibly delete the test in question entirely, because I don't think it's testing anything related to what the file-header comment claims to be testing.In
tiny.clifI guess the test is only supposed to check that the parser can parse lane indexes forextractlaneandinsertlane. Makingv0be ani32x4parameter instead of the result of aniconstshould do that just as well, I think.
Wasmtime GitHub notifications bot (Mar 21 2023 at 01:00):elliottt updated PR #6070 from trevor/restrict-isplit-iconcat to main.
Wasmtime GitHub notifications bot (Mar 21 2023 at 01:19):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2023 at 01:51):elliottt merged PR #6070.
Last updated: Nov 22 2024 at 17:03 UTC