Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace InsertLane format with TernaryImm8 #1762

Closed
wants to merge 1 commit into from

Conversation

abrown
Copy link
Contributor

@abrown abrown commented May 26, 2020

The InsertLane format has an ordering (value().imm().value()) and immediate name ("lane") that make it awkward to use for other instructions. This changes the ordering (value().value().imm()) and uses the default name ("imm") throughout the codebase.

@abrown
Copy link
Contributor Author

abrown commented May 26, 2020

I am also thinking of doing the same for ExtractLane--it's a bit too specific--but that can be a separate PR.

@abrown abrown requested a review from iximeow May 26, 2020 21:14
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. cranelift:wasm labels May 26, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift", "cranelift:meta", "cranelift:wasm"

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

  • bnjbvr: cranelift

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

Learn more.

The InsertLane format has an ordering (`value().imm().value()`) and immediate name (`"lane"`) that make it awkward to use for other instructions. This changes the ordering (`value().value().imm()`) and uses the default name (`"imm"`) throughout the codebase.
abrown added a commit to abrown/wasmtime that referenced this pull request May 27, 2020
Like bytecodealliance#1762, this change the name of the `ExtractLane` format to the more-general `BinaryImm8` and renames its immediate argument from `lane` to `imm`.
@iximeow
Copy link
Contributor

iximeow commented May 29, 2020

I feel it's important to note here that this is a pretty x86-oriented change - do NEON instructions have similar "select-a-lane-of-the-dest-with-an-immediate" instructions? It seems like the alternative is to have different formats for different place imm8 makes sense in clif IR, which is admittedly not great..

edit: NEON instructions would go through the new backend machinery anyway, so that question isn't so relevant.

I don't want to single-handedly encourage orienting the old backend machinery towards x86, so: this seems like a fair change in considering other reg, reg, imm-encoded SIMD instructions but I'd like a second ✔️ on the broader idea of this code really just supporting x86 encoding at this point.

@abrown
Copy link
Contributor Author

abrown commented May 29, 2020

Closing this one in favor of #1770 after chatting offline with @iximeow.

@abrown abrown closed this May 29, 2020
@abrown abrown deleted the ternary-imm8 branch May 29, 2020 22:10
abrown added a commit to abrown/wasmtime that referenced this pull request May 29, 2020
Like bytecodealliance#1762, this change the name of the `ExtractLane` format to the more-general `BinaryImm8` and renames its immediate argument from `lane` to `imm`.
abrown added a commit that referenced this pull request May 30, 2020
Like #1762, this change the name of the `ExtractLane` format to the more-general `BinaryImm8` and renames its immediate argument from `lane` to `imm`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:meta Everything related to the meta-language. cranelift:wasm cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants