Stream: git-wasmtime

Topic: wasmtime / PR #3544 x64: Move `insertlane` to ISLE

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:05):

alexcrichton edited PR #3544 from fix-movsd-issue to main:

This also fixes a bug where movsd was incorrectly used with a memory
operand for insertlane, causing it to actually zero the upper bits
instead of preserving them.

Note that the insertlane logic still exists in lower.rs because it's
used as a helper for a few other instruction lowerings which aren't
migrated to ISLE yet. This commit also adds a helper in ISLE itself for
those other lowerings to use when they get implemented.

Closes #3216

<!--

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 (Nov 18 2021 at 16:48):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:48):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:48):

abrown created PR review comment:

It occurs to me looking at this that I should have originally put a debug_assert in the lowering code to check for correct indexes; I don't really think this PR needs to change but I'm curious--if we did want to check that the index was in range for the type, how would we do so in ISLE?

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:48):

abrown created PR review comment:

:+1:

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:55):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:55):

alexcrichton created PR review comment:

I don't think that a debug_asert! would directly translate but what could be done is to have a different extractor here instead of u8_from_uimm8 but rather something like u{1,2,3,4}_from_uimm8 where the rule wouldn't get matched if the index was out-of-bounds, and then later we'd have a codegen error of "ISLE didn't match this" and manual inspection would show that the lane index was out-of-bounds. @fitzgen may have other ideas though.

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:56):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:56):

abrown created PR review comment:

Ok, cool; thanks, just curious.

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:56):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 16:56):

cfallin created PR review comment:

The right way would probably be to assert in Rust code called by the rule; either in one of the extractors in the pattern (make a specific LaneIndex type then a (lane_index_from_uimm8 idx) extractor pattern that returns it?) or in the constructor of whatever instruction takes it.

We could also have a generic (assert ...) constructor that we could invoke in the right-hand side but that seems like a bit of an antipattern; better to just encode the invariant in the types :-)

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:27):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:27):

fitzgen created PR review comment:

Seconding what @cfallin said :)

Wasmtime GitHub notifications bot (Nov 18 2021 at 17:34):

alexcrichton updated PR #3544 from fix-movsd-issue to main.

Wasmtime GitHub notifications bot (Nov 18 2021 at 19:48):

alexcrichton merged PR #3544.

Last updated: Dec 23 2024 at 12:05 UTC