Skip to content

Winch aarch64 cmp#9018

Merged
saulecabrera merged 5 commits intobytecodealliance:mainfrom
vulc41n:winch-aarch64-cmp
Jul 27, 2024
Merged

Winch aarch64 cmp#9018
saulecabrera merged 5 commits intobytecodealliance:mainfrom
vulc41n:winch-aarch64-cmp

Conversation

@vulc41n
Copy link
Copy Markdown
Contributor

@vulc41n vulc41n commented Jul 26, 2024

This PR implements wasm comparison instructions for winch targeting aarch64.
#8321

@vulc41n vulc41n requested review from a team as code owners July 26, 2024 12:01
@vulc41n vulc41n requested review from cfallin and pchickey and removed request for a team July 26, 2024 12:01
@saulecabrera
Copy link
Copy Markdown
Member

saulecabrera commented Jul 26, 2024

I can help reviewing this change.

@saulecabrera saulecabrera requested review from saulecabrera and removed request for cfallin and pchickey July 26, 2024 14:25
saulecabrera
saulecabrera reviewed Jul 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

Sorry it took me a bit to review this change. In general it looks great to me, thanks. I left one nit below which I think it makes sense to address before merging.

saulecabrera
saulecabrera approved these changes Jul 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

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

Thanks!

@vulc41n
Copy link
Copy Markdown
Contributor Author

vulc41n commented Jul 27, 2024

No problem, thanks for reviewing my pull request 🙂
I've removed rd from subs methods, but I do see Wd in the doc you linked. Am I missing something ?
Please tell me if there is anything else I can do to improve this

@saulecabrera saulecabrera added this pull request to the merge queue Jul 27, 2024
@saulecabrera
Copy link
Copy Markdown
Member

saulecabrera commented Jul 27, 2024

No problem, thanks for reviewing my pull request 🙂 I've removed rd from subs methods, but I do see Wd in the doc you linked. Am I missing something ? Please tell me if there is anything else I can do to improve this

Yeah, according to the docs there's a proper destination register, however, it's not currently used in the way we lower comparison operations in Winch, since we rely on the flags or in cmp_with_set, so my suggestion is mostly to make it easier to reflect the usage.

Merged via the queue into bytecodealliance:main with commit c549e77 Jul 27, 2024
@vulc41n vulc41n deleted the winch-aarch64-cmp branch July 27, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saulecabrera saulecabrera saulecabrera approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vulc41n @saulecabrera