Fix some 16- and 8-bit behavior in x64 backend related to rotates.#3610
Merged
Conversation
Uncovered by @bjorn3 (thanks!): 8- and 16-bit rotates were not working properly in recent versions of Cranelift with part of the lowering migrated to ISLE. This PR fixes a few issues: - 8- and 16-bit rotate-left needs to mask a constant amount, if any, because we use a 32-bit rotate instruction and so don't get the appropriate shift-amount masking for free from x86 semantics. - `operand_size_from_type` was incorrect: it only handled 32- and 64-bit types and silently returned `OperandSize::Size32` for everything else. Now uses the `OperandSize::from_ty(ty)` helper as the pre-ISLE code did. Our test coverage for narrow value types is not great; this PR adds some runtests for rotl/rotr but more would always be better!
github-actions
Bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "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 |
alexcrichton
approved these changes
Dec 16, 2021
…e-width for shifts.
Contributor
|
I can confirm this fixes rotate in cg_clif. |
Merged
bjorn3
added a commit
to rust-lang/rustc_codegen_cranelift
that referenced
this pull request
Dec 17, 2021
This reverts commit a1037fa. There are two regressions in Cranelift with respect to small integer operations. Both have already been fixed on thebmain branch, but we will have to wait for a new Cranelift release. They have been fixed by bytecodealliance/wasmtime#3610 and bytecodealliance/wasmtime#3617.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uncovered by @bjorn3 (thanks!): 8- and 16-bit rotates were not working
properly in recent versions of Cranelift with part of the lowering
migrated to ISLE.
This PR fixes a few issues:
8- and 16-bit rotate-left needs to mask a constant amount, if any,
because we use a 32-bit rotate instruction and so don't get the
appropriate shift-amount masking for free from x86 semantics.
operand_size_from_typewas incorrect: it only handled 32- and 64-bittypes and silently returned
OperandSize::Size32for everything else.Now uses the
OperandSize::from_ty(ty)helper as the pre-ISLE codedid.
Our test coverage for narrow value types is not great; this PR adds some
runtests for rotl/rotr but more would always be better!