Stream: git-wasmtime

Topic: wasmtime / PR #9018 Winch aarch64 cmp


view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2024 at 12:01):

vulc41n opened PR #9018 from vulc41n:winch-aarch64-cmp to bytecodealliance:main:

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

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2024 at 12:01):

vulc41n requested cfallin for a review on PR #9018.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2024 at 12:01):

vulc41n requested wasmtime-compiler-reviewers for a review on PR #9018.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2024 at 12:01):

vulc41n requested pchickey for a review on PR #9018.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2024 at 12:01):

vulc41n requested wasmtime-core-reviewers for a review on PR #9018.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2024 at 14:25):

saulecabrera commented on PR #9018:

I can help reviewing this change.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2024 at 14:25):

saulecabrera requested saulecabrera for a review on PR #9018.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2024 at 13:59):

saulecabrera submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2024 at 13:59):

saulecabrera submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2024 at 13:59):

saulecabrera created PR review comment:

I'm thinking that it might make sense to drop the rd param in this case, to match more accurately the instruction definition and the method usage and use regs::zero internally when emitting emit_alu_rrr_extend and emit_alu_rri. That would simplify the usage from the MacroAssembler

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2024 at 15:08):

vulc41n updated PR #9018.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2024 at 15:18):

saulecabrera submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2024 at 15:18):

vulc41n commented on PR #9018:

No problem, thanks for reviewing my pull request :smile:
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

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2024 at 15:26):

saulecabrera commented on PR #9018:

No problem, thanks for reviewing my pull request :smile: 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2024 at 15:33):

saulecabrera merged PR #9018.


Last updated: Dec 23 2024 at 12:05 UTC