Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Implement splat instruction for x86 #833

Merged
merged 10 commits into from
Jul 17, 2019
Merged

Implement splat instruction for x86 #833

merged 10 commits into from
Jul 17, 2019

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Jul 8, 2019 •
edited
Loading

This is a work in progress and not ready to merge but I am opening the PR for discussion (see #803). I will highlight my top questions in the code below.

[edit] This should be ready for a real review now; I've resolved all of the discussion issues.

abrown
abrown commented Jul 8, 2019
View reviewed changes
filetests/isa/x86/pinsr.clif Outdated Show resolved Hide resolved
@abrown
Copy link
Collaborator Author

abrown commented Jul 9, 2019

@sunfishcode, one other thought: is there a way to guard this feature under some flag until we can properly test that all of the encodings are correct by compiling and running some WASM code?

@sunfishcode
Copy link
Member

@sunfishcode, one other thought: is there a way to guard this feature under some flag until we can properly test that all of the encodings are correct by compiling and running some WASM code?

We could use the enable_simd setting for this. It's defaulted to true right now, but we could set it to false while the SIMD code is under development to prevent frontends from accidentally using it.

@abrown abrown changed the title WIP: Implement splat instruction for x86 Implement splat instruction for x86 Jul 10, 2019
@abrown
Copy link
Collaborator Author

abrown commented Jul 10, 2019

@bnjbvr, @sunfishcode, @bjorn3: this should be ready for a real review now. Do I need to rebase and squash or will you squash when you merge from here?

@bnjbvr
Copy link
Member

bnjbvr commented Jul 11, 2019

Nice. I would personally like it if there was a way to squash some of your commits together, so:

  • each represent a unit of work
  • that properly compiles and passes tests

This might be hard to do after the fact, but in some cases it's possible to do so, which would help make the review much simpler. Does it make sense to you to try to do so?

@abrown
Prepare legalizer codegen for SIMD features
673c6e5 
Contains fixes from @bnjbvr to codegen as a part of bnjbvr#2; necessary for SIMD features to work
bjorn3
bjorn3 suggested changes Jul 11, 2019
View reviewed changes
cranelift-codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved
@abrown
Add x86-specific SIMD settings, e.g. SSE2
076b398 
Also, ties SIMD ISA predicates to the shared enable_simd setting
@abrown
Copy link
Collaborator Author

abrown commented Jul 11, 2019

@bjorn3's comments are addressed (do you have more?) and I rebased the commits to something that should make more sense in a git history, running test-all.sh after each. I do have one pending question about argument assignment (above) if anyone has an opinion.

bjorn3
bjorn3 reviewed Jul 12, 2019
View reviewed changes
cranelift-codegen/meta/src/isa/x86/encodings.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/encodings.rs Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/encodings.rs Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/legalize.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/legalize.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/legalize.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/legalize.rs Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/legalize.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/legalize.rs Show resolved Hide resolved
cranelift-codegen/meta/src/shared/instructions.rs Show resolved Hide resolved
bjorn3
bjorn3 reviewed Jul 12, 2019
View reviewed changes
cranelift-codegen/meta/src/cdsl/types.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/encodings.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/legalize.rs Outdated Show resolved Hide resolved
cranelift-codegen/meta/src/isa/x86/recipes.rs Show resolved Hide resolved
cranelift-codegen/meta/src/shared/instructions.rs Show resolved Hide resolved
cranelift-codegen/src/ir/immediates.rs Outdated Show resolved Hide resolved
@abrown abrown force-pushed the add-splat-x86 branch 3 times, most recently from a36cffa to 03c7803 Compare July 12, 2019 17:16
bjorn3
bjorn3 approved these changes Jul 12, 2019
View reviewed changes
Copy link
Contributor

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

LGTM

@abrown
Assign vector arguments to FPR registers
6260290
@abrown
Allow CDSL instructions to bind to vector types
9f67d91
@abrown
Add scalar_to_vector instruction
a004fe0 
Moves scalar values in a GPR register to an FPR register
@abrown
Add raw_bitcast instruction
bf55818 
Casts bits as a different type of the same width with no change to the data (unlike bitcast)
@abrown
Add x86-specific shuffle instructions
d009146 
This includes both PSHUFD and PSHUFB; these are necessary to legalize future SIMD instructions.
@abrown
Add x86 implementation of insertlane instruction
5f0aeaf
@abrown
Add x86 implementation of splat instruction
ec1c837
sunfishcode
sunfishcode approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Looks good! And thanks so much @bjorn3 for the review here!

@sunfishcode sunfishcode merged commit a5b17e8 into bytecodealliance:master Jul 17, 2019
@abrown abrown deleted the add-splat-x86 branch July 17, 2019 00:08
@abrown abrown mentioned this pull request Jul 17, 2019
Merged
@bjorn3 bjorn3 mentioned this pull request Jul 27, 2019
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bnjbvr bnjbvr bnjbvr left review comments

@bjorn3 bjorn3 bjorn3 approved these changes

@sunfishcode sunfishcode sunfishcode approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abrown @sunfishcode @bnjbvr @bjorn3