Skip to content

x64: Convert compare instructions to the new assembler #10836

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

Merged

Conversation

rahulchaphalkar
Copy link
Contributor

Implement compare instructions.
Implement Eflags logic for DSL and wire up isle rules to set flags when appropriate.

@rahulchaphalkar rahulchaphalkar requested a review from a team as a code owner May 27, 2025 02:47
@rahulchaphalkar rahulchaphalkar requested review from abrown and removed request for a team May 27, 2025 02:47
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. winch Winch issues or pull requests labels May 27, 2025
Copy link

Subscribe to Label Action

cc @saulecabrera

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

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

  • saulecabrera: winch

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

Learn more.

@abrown abrown self-assigned this May 28, 2025
Copy link
Member

@abrown abrown left a comment

Choose a reason for hiding this comment

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

I think this makes sense; the only remaining refactoring is to unwrap some unnecessary Inst::* code and then this should be good-to-go.

@rahulchaphalkar rahulchaphalkar force-pushed the compare-instructions-rebase branch from 17cbb3c to 71f52df Compare June 9, 2025 19:18
@rahulchaphalkar
Copy link
Contributor Author

@abrown
I have rebased this on the Custom logic PR as well as latest main.
For compare instructions, we need to change operands as well (remove the immediate) as well as change name, so now we are passing ordered_ops string to custom::Display functions, which returns a concatenated string containing name and ordered_ops.

@rahulchaphalkar rahulchaphalkar force-pushed the compare-instructions-rebase branch from 002e205 to 95df59e Compare June 9, 2025 22:07
@rahulchaphalkar rahulchaphalkar force-pushed the compare-instructions-rebase branch from 339b08d to ee21774 Compare June 11, 2025 16:32
abrown added a commit to abrown/wasmtime that referenced this pull request Jun 11, 2025
Sometimes we just want to fix up the printed name of the instruction in
the disassembly; e.g., instructions with the `lock` prefix. But, as
we're seeing in bytecodealliance#10836 and elsewhere, other times we want to control the
printing of the entire instruction--the full `Display` implementation.
We'll need this kind of thing if we want to swap the order of operands
or omit an immediate if it ends up being included in the printed
instruction name.

This change splits this kind of customization in two: `Mnemonic` and
`Display`. After renaming this enum to `Customization`, all existing
uses of `Display` are migrated to `Mnemonic`.
Copy link
Member

@abrown abrown left a comment

Choose a reason for hiding this comment

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

LGTM! I think the one last thing we'll want is to clean up the ordered_ops bits once #11017 is merged.

github-merge-queue bot pushed a commit that referenced this pull request Jun 11, 2025
Sometimes we just want to fix up the printed name of the instruction in
the disassembly; e.g., instructions with the `lock` prefix. But, as
we're seeing in #10836 and elsewhere, other times we want to control the
printing of the entire instruction--the full `Display` implementation.
We'll need this kind of thing if we want to swap the order of operands
or omit an immediate if it ends up being included in the printed
instruction name.

This change splits this kind of customization in two: `Mnemonic` and
`Display`. After renaming this enum to `Customization`, all existing
uses of `Display` are migrated to `Mnemonic`.
@rahulchaphalkar rahulchaphalkar force-pushed the compare-instructions-rebase branch from ee21774 to cd27e90 Compare June 12, 2025 19:17
@rahulchaphalkar
Copy link
Contributor Author

@abrown ok i have rebased and pushed with changes, CI still running.

@abrown abrown added this pull request to the merge queue Jun 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jun 12, 2025
@rahulchaphalkar rahulchaphalkar force-pushed the compare-instructions-rebase branch from f749f7c to f0f1a71 Compare June 12, 2025 20:58
@abrown abrown enabled auto-merge June 12, 2025 21:11
@abrown abrown added this pull request to the merge queue Jun 12, 2025
Merged via the queue into bytecodealliance:main with commit cd1615d Jun 12, 2025
160 checks passed
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:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants