Skip to content

Replace InsertLane format with TernaryImm8#1762

Closed
abrown wants to merge 1 commit intobytecodealliance:masterfrom
abrown:ternary-imm8
Closed

Replace InsertLane format with TernaryImm8#1762
abrown wants to merge 1 commit intobytecodealliance:masterfrom
abrown:ternary-imm8

Conversation

@abrown
Copy link
Member

@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
Member 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

Details 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
Member 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