elliottt opened PR #4625 from trevor/x64-icmp-bool-opt
to main
:
- Add tests asserting existing behavior for
icmp slt
- Add a peephole optimization for
x < 0
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt edited PR #4625 from trevor/x64-icmp-bool-opt
to main
:
Fixes #4607
Add a peephole optimization for
icmp slt x, (iconst 0)
for 32 and 64 bit values, turning them into a right shift by 31 or 63 bits respectively. This optimization only applies to the case where the result of theicmp
is used directly, cases likebrz (icmp ..)
will be left alone.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin submitted PR review.
elliottt updated PR #4625 from trevor/x64-icmp-bool-opt
to main
.
MaxGraey submitted PR review.
MaxGraey created PR review comment:
Hmm, why it uses signed right shift now? It should be an unsigned shift
MaxGraey submitted PR review.
MaxGraey created PR review comment:
Is it correct? I guess it should be:
notl %edi, %edi shrl $31, %edi, %edi
MaxGraey submitted PR review.
MaxGraey created PR review comment:
Doesn't ISLE and cranelift ir always canonicalize constants on the right?
MaxGraey edited PR review comment.
MaxGraey edited PR review comment.
MaxGraey edited PR review comment.
MaxGraey edited PR review comment.
MaxGraey edited PR review comment.
elliottt submitted PR review.
elliottt created PR review comment:
This was with an eye towards better supporting our boolean representations, as mentioned in #3205, as
sar
will do sign extension.
elliottt created PR review comment:
I've inverted it and restricted this optimization to boolean-producing instructions. Also this shouldn't affect the
test/jmp
fusion, as this optimization will only fire foricmp
instructions whose results are used directly, not as the input to a branch decision. Was there another fusion case you were thinking this might invalidate?
elliottt submitted PR review.
elliottt created PR review comment:
Not currently, hence the symmetric cases for this optimization. This is something we might look into after the mid-end optimization work has landed.
elliottt submitted PR review.
MaxGraey submitted PR review.
MaxGraey created PR review comment:
I meant macro / micro fusion on the CPU side
MaxGraey deleted PR review comment.
MaxGraey submitted PR review.
MaxGraey created PR review comment:
Got it. Thanks!
MaxGraey submitted PR review.
MaxGraey created PR review comment:
I see
elliottt updated PR #4625 from trevor/x64-icmp-bool-opt
to main
.
elliottt created PR review comment:
Given that it's not clear to me that we can use the wider boolean types with
icmp
, I've switched back toshr
with a type guard on the result. Sorry about the back and forth here!
elliottt submitted PR review.
elliottt has marked PR #4625 as ready for review.
elliottt requested cfallin for a review on PR #4625.
cfallin submitted PR review.
elliottt merged PR #4625.
Last updated: Dec 23 2024 at 13:07 UTC