elliottt opened issue #5869:
Feature
The x64 backend has a helper named
emit_cmp
ininst.isle
that is used when emitting comparisons. Currently it will always usex64_cmp
to emit the comparison, but for cases of equality comparisons with0
,x64_test
would be a better choice. Additionally, theselect
lowering does not useemit_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 oficmp
in the x64 backend.
elliottt labeled issue #5869:
Feature
The x64 backend has a helper named
emit_cmp
ininst.isle
that is used when emitting comparisons. Currently it will always usex64_cmp
to emit the comparison, but for cases of equality comparisons with0
,x64_test
would be a better choice. Additionally, theselect
lowering does not useemit_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 oficmp
in the x64 backend.
elliottt labeled issue #5869:
Feature
The x64 backend has a helper named
emit_cmp
ininst.isle
that is used when emitting comparisons. Currently it will always usex64_cmp
to emit the comparison, but for cases of equality comparisons with0
,x64_test
would be a better choice. Additionally, theselect
lowering does not useemit_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 oficmp
in the x64 backend.
elliottt labeled issue #5869:
Feature
The x64 backend has a helper named
emit_cmp
ininst.isle
that is used when emitting comparisons. Currently it will always usex64_cmp
to emit the comparison, but for cases of equality comparisons with0
,x64_test
would be a better choice. Additionally, theselect
lowering does not useemit_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 oficmp
in the x64 backend.
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'sselect
instruction on x86.
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
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))
cfallin commented on issue #5869:
@Solo-steven your refactor looks reasonable -- we'd be happy to review a PR with this. Thanks!
sudoHackIn commented on issue #5869:
Hi, @cfallin is this issue still actual or should be closed after Solo-steven's pr is merged?
cfallin commented on issue #5869:
@sudoHackIn the PR above addresed the second bit (
select
using the helper) but not the first (usingtest
rather thancmp
), so this issue should remain open, I think.
Last updated: Dec 23 2024 at 12:05 UTC