refactor: using emit_cmp helper to refactor lowering of selection ins…#7344
refactor: using emit_cmp helper to refactor lowering of selection ins…#7344cfallin merged 2 commits intobytecodealliance:mainfrom
Conversation
github-actions
bot
added
cranelift
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks! One formatting nit below, otherwise this seems correct.
Do you expect codegen to improve for any combination of compare and select instructions? If so, could we add a test showing that?
| (rule (lower (has_type ty (select (maybe_uextend (icmp cc a @ (value_type (fits_in_64 a_ty)) b)) x y))) | ||
| (let ((size OperandSize (raw_operand_size_of_type a_ty))) | ||
| (with_flags (x64_cmp size b a) (cmove_from_values ty cc x y)))) | ||
| (lower_select_icmp ty (emit_cmp cc a b ) x y)) |
There was a problem hiding this comment.
formatting nit: remove the space between b and the right-paren?
|
Hi @cfallin thanks for your review, I think this change will behave the same as before, just a simple refactor to using Because I am new to both ISLE and clif IR, I might be missing some potential improvements we can make. if so please point it out to me , thanks ~ |
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Looks good, thanks! I didn't necessarily expect any codegen changes either, I was just curious if there were any intended. This looks like a pure refactor which is perfectly fine!
2 participants
This PR implements the second part of issue #5869 .
As lowering the
selectinstruction based onfcmp. I declare a helper function for loweringicmp-based select instruction.