Stream: git-wasmtime

Topic: wasmtime / issue #5869 Add special cases in the x64 `emit...

Wasmtime GitHub notifications bot (Feb 23 2023 at 19:06):

elliottt opened issue #5869:

Feature

The x64 backend has a helper named emit_cmp in inst.isle that is used when emitting comparisons. Currently it will always use x64_cmp to emit the comparison, but for cases of equality comparisons with 0, x64_test would be a better choice. Additionally, the select lowering does not use emit_cmp right now, and could be refactored to do so with this change in place.

Benefit

This will generate better code for cases where emit_cmp is used to compile a use of icmp in the x64 backend.

Wasmtime GitHub notifications bot (Feb 23 2023 at 19:06):

elliottt labeled issue #5869:

Feature

The x64 backend has a helper named emit_cmp in inst.isle that is used when emitting comparisons. Currently it will always use x64_cmp to emit the comparison, but for cases of equality comparisons with 0, x64_test would be a better choice. Additionally, the select lowering does not use emit_cmp right now, and could be refactored to do so with this change in place.

Benefit

This will generate better code for cases where emit_cmp is used to compile a use of icmp in the x64 backend.

Wasmtime GitHub notifications bot (Feb 23 2023 at 19:06):

elliottt labeled issue #5869:

Feature

The x64 backend has a helper named emit_cmp in inst.isle that is used when emitting comparisons. Currently it will always use x64_cmp to emit the comparison, but for cases of equality comparisons with 0, x64_test would be a better choice. Additionally, the select lowering does not use emit_cmp right now, and could be refactored to do so with this change in place.

Benefit

This will generate better code for cases where emit_cmp is used to compile a use of icmp in the x64 backend.

Wasmtime GitHub notifications bot (Feb 23 2023 at 19:06):

elliottt labeled issue #5869:

Feature

The x64 backend has a helper named emit_cmp in inst.isle that is used when emitting comparisons. Currently it will always use x64_cmp to emit the comparison, but for cases of equality comparisons with 0, x64_test would be a better choice. Additionally, the select lowering does not use emit_cmp right now, and could be refactored to do so with this change in place.

Benefit

This will generate better code for cases where emit_cmp is used to compile a use of icmp in the x64 backend.

Wasmtime GitHub notifications bot (Apr 17 2023 at 15:50):

jameysharp commented on issue #5869:

The first part of this issue (using test for comparisons with 0) was finished in #6086, but there's still work to do with simplifying the lowering for Cranelift's select instruction on x86.

Wasmtime GitHub notifications bot (Oct 21 2023 at 11:03):

Solo-steven commented on issue #5869:

Hi @jameysharp I am interested in this issue, I am a beginner to both compiler backend and isle, so I would like to discuss my solution there before I open PR ~

I think the second part of the issue can be solved by adding a helper for lowering select inst.

(decl lower_select_icmp (Type IcmpCondResult Value Value) InstOutput)
(rule (lower_select_icmp ty (IcmpCondResult.Condition flags cc) x y)
      (with_flags flags (cmove_from_values ty cc x y)))

I think we can change the existing rule to use the helper above, form existed code

https://github.com/bytecodealliance/wasmtime/blob/ed68661976554b8ea967d7d3ca2f2e0741323aad/cranelift/codegen/src/isa/x64/lower.isle#L2084

to use helper above :

(rule (lower (has_type ty (select (maybe_uextend (icmp cc a @ (value_type (fits_in_64 a_ty)) b)) x y)))
      (lower_select_icmp ty (emit_cmp cc a b ) x y))

Wasmtime GitHub notifications bot (Oct 23 2023 at 16:46):

cfallin commented on issue #5869:

@Solo-steven your refactor looks reasonable -- we'd be happy to review a PR with this. Thanks!

Wasmtime GitHub notifications bot (Sep 10 2024 at 21:51):

sudoHackIn commented on issue #5869:

Hi, @cfallin is this issue still actual or should be closed after Solo-steven's pr is merged?

Wasmtime GitHub notifications bot (Sep 10 2024 at 21:55):

cfallin commented on issue #5869:

@sudoHackIn the PR above addresed the second bit (select using the helper) but not the first (using test rather than cmp), so this issue should remain open, I think.

Wasmtime GitHub notifications bot (Feb 04 2025 at 22:48):

erikrose commented on issue #5869:

Would anyone mind if I have a try at the select half of this?

Wasmtime GitHub notifications bot (Feb 04 2025 at 22:58):

cfallin commented on issue #5869:

@erikrose the select bit was already handled (see comment above yours) but it would be great to use test to test against zero -- test rN, rNshould generate shorter code than cmp rN, 0 with our unconditional 32-bit immediate usage.

Wasmtime GitHub notifications bot (Feb 07 2025 at 14:41):

erikrose commented on issue #5869:

@sudoHackIn the PR above addresed the second bit (select using the helper) but not the first (using test rather than cmp), so this issue should remain open, I think.

As per https://github.com/bytecodealliance/wasmtime/issues/5869#issuecomment-1511633918 (and looking at https://github.com/bytecodealliance/wasmtime/blob/d943d57e78950da21dd430e0847f3b8fd0ade073/cranelift/codegen/src/isa/x64/inst.isle#L5192-L5201), I believe this whole thing is done. Right, @cfallin?

Wasmtime GitHub notifications bot (Feb 07 2025 at 17:06):

cfallin closed issue #5869:

Feature

The x64 backend has a helper named emit_cmp in inst.isle that is used when emitting comparisons. Currently it will always use x64_cmp to emit the comparison, but for cases of equality comparisons with 0, x64_test would be a better choice. Additionally, the select lowering does not use emit_cmp right now, and could be refactored to do so with this change in place.

Benefit

This will generate better code for cases where emit_cmp is used to compile a use of icmp in the x64 backend.

Wasmtime GitHub notifications bot (Feb 07 2025 at 17:06):

cfallin commented on issue #5869:

Ah, indeed, I believe this is fully done then and we can close it. Thanks for connecting the dots!

Last updated: Feb 28 2025 at 02:27 UTC