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

x64: Migrate fcvt_from_sint and fcvt_low_from_sint to ISLE #4650

Merged
merged 8 commits into from
Aug 10, 2022

Conversation

elliottt
Copy link
Member

@elliottt elliottt commented Aug 9, 2022

Migrate fcvt_from_sint and fcvt_low_from_sint to ISLE. Also support the Cvtdq2ps opcode when emitting XmmUnaryRmR instructions.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Aug 9, 2022
@elliottt elliottt marked this pull request as ready for review August 9, 2022 03:35
@elliottt elliottt added the isle Related to the ISLE domain-specific language label Aug 9, 2022
@github-actions
Copy link

github-actions bot commented Aug 9, 2022

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "isle"

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

  • cfallin: isle
  • fitzgen: isle

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

Learn more.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks! Just one note below as we're going through the opcodes to fully clean up the unary vs binary distinction...

@@ -1527,6 +1527,7 @@ pub(crate) fn emit(
SseOpcode::Cvtdq2pd => (LegacyPrefixes::_F3, 0x0FE6, 2),
SseOpcode::Cvtpd2ps => (LegacyPrefixes::_66, 0x0F5A, 2),
SseOpcode::Cvtps2pd => (LegacyPrefixes::None, 0x0F5A, 2),
SseOpcode::Cvtdq2ps => (LegacyPrefixes::None, 0x0F5B, 2),
Copy link
Member

Choose a reason for hiding this comment

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

In addition to adding it here, can we remove it from here and make sure we switch over any uses of that (tweaking any handwritten uses of it in-place is fine for now)? Since the opcode has unary (one src, one dst) semantics it should only appear here, not elsewhere.

I'm also slightly suspect of the two cvt* opcodes above that one in XmmRmR too, and we should see if they should be moved over too, while we're at it...

@elliottt elliottt merged commit a25d520 into bytecodealliance:main Aug 10, 2022
@elliottt elliottt deleted the trevor/x64-fcvt branch August 10, 2022 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants