wasmtime / PR #9018 Winch aarch64 cmp · git-wasmtime

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.

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.

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

Wasmtime GitHub notifications bot (Jul 27 2024 at 15:08):

vulc41n updated PR #9018.

Wasmtime GitHub notifications bot (Jul 27 2024 at 15:18):

saulecabrera submitted PR review:

Thanks!

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

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.

Wasmtime GitHub notifications bot (Jul 27 2024 at 15:33):

saulecabrera merged PR #9018.

Last updated: Nov 22 2024 at 17:03 UTC