x64: Move insertlane to ISLE#3544
Conversation
github-actions
bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
|
||
| ;;;; Rules for `insertlane` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; | ||
|
|
||
| (rule (lower (insertlane vec @ (value_type ty) val (u8_from_uimm8 idx))) |
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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 :-)
| ;; two `movsd` instructions. The first `movsd` (used as `xmm_unary_rm_r`) will | ||
| ;; load from memory into a temp register and then the second `movsd` (modeled | ||
| ;; internally as `xmm_rm_r` will merge the temp register into our `vec` | ||
| ;; register. |
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 bytecodealliance#3216
alexcrichton
force-pushed
the
fix-movsd-issue
branch
from
ac62e07 to
a2c9a6c
Compare
github-actions
bot
added
cranelift:area:machinst
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 bytecodealliance#3216
4 participants
This also fixes a bug where
movsdwas incorrectly used with a memoryoperand for
insertlane, causing it to actually zero the upper bitsinstead of preserving them.
Note that the insertlane logic still exists in
lower.rsbecause it'sused 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