Skip to content

cranelift: Align Scalar and SIMD shift semantics#4520

Merged
jameysharp merged 5 commits intobytecodealliance:mainfrom
afonso360:vec-shift
Jul 27, 2022
Merged

cranelift: Align Scalar and SIMD shift semantics#4520
jameysharp merged 5 commits intobytecodealliance:mainfrom
afonso360:vec-shift

Conversation

@afonso360
Copy link
Contributor

@afonso360 afonso360 commented Jul 24, 2022

👋 Hey,

In #4424 @uweigand reported that we have different semantics for scalar and SIMD shifts, which is unusual in our instruction set.

This PR corrects the AArch64 and x86 backends to mask the shift amount before performing SIMD shifts. The s390x backend should already follow this behavior.

It also reorganizes the ishl/ushl/sshl tests into separate files. Since we had a bunch of duplicate tests in hard to find files that were somewhat unrelated.

I tried to keep changes contained in separate commits so that it's easier to review.

Fixes #4424
CC: @cfallin @abrown @uweigand @akirilov-arm

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:wasm isle Related to the ISLE domain-specific language labels Jul 24, 2022
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

Details This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64", "cranelift:wasm", "isle"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the way you split up these commits! I wish I could ask CI to test after each commit, but it looks plausible to me that the tests would all pass at every step.

I'm not comfortable reviewing the aarch64 and x64 changes with any confidence. They look reasonable to me but I think others should review them to be sure.

I think your changes to the runtests look great, with a nice range of tests to cover this behavior. And the cranelift-wasm changes seem obviously correct.

Last thing I'm wondering is whether this will affect any other users of Cranelift, like @bjorn3.

Group some SIMD operations by instruction.
Also, new tests with the mod behaviour
Copy link
Contributor

@akirilov-arm akirilov-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AArch64 changes look good to me, with just a tiny nit.

Copy link
Contributor

@akirilov-arm akirilov-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone else should have a look at the x64 bits.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Just a few nits below. The final payoff in simpler code in the Wasm translator is great to see.

@jameysharp jameysharp enabled auto-merge (squash) July 27, 2022 17:32
@jameysharp jameysharp merged commit 0508932 into bytecodealliance:main Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:wasm cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Craneline vector shift count semantics does not match docs

4 participants