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

Make formats less SIMD-specific #1770

Merged
merged 3 commits into from
May 30, 2020
Merged

Conversation

abrown
Copy link
Contributor

@abrown abrown commented May 27, 2020

This renames and slightly alters several formats:

  • InsertLane becomes TernaryImm8
  • ExtractLane becomes BinaryImm8
  • BinaryImm becomes BinaryImm64 for consistency

This has additional comments in #1762 about the ordering of operands; in changing InsertLane to TernaryImm8 I changed the order of the operands in the CLIF IR (value, imm, value to value, value, imm) so that this format will make more sense for other instructions (x86_pshufd, x86_pblendw). Other than that this should be a straightforward renaming.

A heads up at the Cranelift API level: users of Cranelift IR (e.g. @bjorn3, @jyn514?) might notice this naming chaning if they are using Format or InstructionData directly.

@abrown
Copy link
Contributor Author

abrown commented May 27, 2020

Waiting on #1762...

@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 27, 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.

@jyn514
Copy link
Contributor

jyn514 commented May 27, 2020

I am not using the textual IR for anything besides debugging, so no complaints from me :)

@github-actions
Copy link

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "cranelift:area:peepmatic"

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

  • fitzgen: cranelift:area:peepmatic

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

Learn more.

@bjorn3
Copy link
Contributor

bjorn3 commented May 27, 2020

I am currently using neither insertlane nor extractlane. I also don't use Format or InstructionData either.

@abrown abrown marked this pull request as ready for review May 29, 2020 21:49
@abrown abrown requested a review from iximeow May 29, 2020 22:07
abrown added 3 commits May 29, 2020 15:13
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.
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 abrown changed the title Replace ExtractLane format with BinaryImm8 Make formats less SIMD-specific May 29, 2020
Copy link
Contributor

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

This look good! and learning why it is the way it is has been informative:

(InsertLane) Why change op1, op1-mod, op2-style IR to op1, op2, op1-mod-style?

Because some instructions, like x86's pshufw, would want a three-arg-one-immediate format, but it would be src, imm, dest, where there's no relation between imm and any operand other than the shuffling pshufw actually performs. "philosophically speaking" in how Cranelift IR is defined, existing formats just aren't quite a good fit.

(InsertLane) Why not have both op1, op1-mod, op2 and op1, op2, mod formats?

The instruction format machinery is lax about where immediates show up. This means that Cranelift can't disceren between op, imm, op and op, op, imm formats

Consequently, having formats that are less SIMD-oriented makes sense both for insert/extract and other instructions. And maybe one unlikely day we'll reuse the ternary format for 3-arg imul :)

edit: unwelcome CI failure though, with no logs?

@abrown abrown merged commit 0dd77d3 into bytecodealliance:master May 30, 2020
@abrown
Copy link
Contributor Author

abrown commented May 30, 2020

unwelcome CI failure though, with no logs?

Yeah, I have no idea. I re-ran them and everything was fine 🤷.

@abrown abrown deleted the binary-imm8 branch May 30, 2020 02:57
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.

4 participants