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

Add x86 shuffle implemention#974

Merged
abrown merged 3 commits into
bytecodealliance:masterfrom
abrown:add-shuffle-x86-on-improved-uimm128
Sep 19, 2019
Merged

Add x86 shuffle implemention#974
abrown merged 3 commits into
bytecodealliance:masterfrom
abrown:add-shuffle-x86-on-improved-uimm128

Conversation

@abrown

@abrown abrown commented Sep 4, 2019

Copy link
Copy Markdown
Member

This uses work pending review in #943 and adds some encodings for boolean comparisons in order to allow more complete testing of the shuffle implementation.

@abrown abrown force-pushed the add-shuffle-x86-on-improved-uimm128 branch 2 times, most recently from e90f9a3 to b961937 Compare September 10, 2019 18:09

@bnjbvr bnjbvr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I only looked at the first commit. I wanted to ask about the second one: why do we need all these new encodings for boolean types?

Looking great otherwise, this is on the right track. (Bonus points if you can split the Immediate -> Constant renaming from the first patch, to make it slightly easier to review; don't worry if this is too much hassle.)

Comment thread cranelift-wasm/src/code_translator.rs Outdated
Comment thread cranelift-serde/src/serde_clif_json.rs Outdated
Comment thread cranelift-codegen/meta/src/shared/formats.rs Outdated
Comment thread cranelift-codegen/meta/src/shared/immediates.rs Outdated
Comment thread cranelift-codegen/meta/src/shared/immediates.rs Outdated
Comment thread cranelift-codegen/src/isa/x86/enc_tables.rs Outdated
Comment thread cranelift-codegen/src/isa/x86/enc_tables.rs Outdated
Comment thread cranelift-codegen/src/isa/x86/enc_tables.rs Outdated
Comment thread cranelift-codegen/src/isa/x86/enc_tables.rs Outdated
Comment thread cranelift-codegen/src/isa/x86/enc_tables.rs Outdated
@abrown

abrown commented Sep 13, 2019

Copy link
Copy Markdown
Member Author

I added the boolean encodings solely in order to write a binary shuffle test: https://github.com/CraneStation/cranelift/blob/27d809a444c6030f4f929153c9e9d5f7456c98a2/filetests/isa/x86/shuffle-run.clif#L47-L58. I can move that to a separate PR but I guess I felt at the time I wrote this that because the binary encodings exactly matched the pre-existing integer encodings the change was relatively safe.

@bnjbvr bnjbvr self-requested a review September 19, 2019 12:51

@bnjbvr bnjbvr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great, thank you very much! Thanks for explaining the boolean encodings, it makes sense.

Comment thread cranelift-codegen/meta/src/shared/instructions.rs Outdated
Comment thread cranelift-codegen/meta/src/shared/instructions.rs Outdated
Comment thread cranelift-codegen/meta/src/shared/instructions.rs Outdated
Comment thread cranelift-codegen/src/isa/x86/enc_tables.rs
Comment thread cranelift-codegen/src/isa/x86/enc_tables.rs
Comment thread cranelift-wasm/src/code_translator.rs Outdated
Comment thread filetests/isa/x86/shuffle-compile.clif Outdated
Includes and, or, xor, not, and regmove; TODO re-factor PerCpuModeEncodings to avoid code duplication
@abrown abrown force-pushed the add-shuffle-x86-on-improved-uimm128 branch from 27d809a to ae00af1 Compare September 19, 2019 17:20
@abrown abrown merged commit ee9a677 into bytecodealliance:master Sep 19, 2019
@abrown abrown deleted the add-shuffle-x86-on-improved-uimm128 branch September 19, 2019 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants