Wasmtime GitHub notifications bot (Aug 05 2022 at 18:22): Wasmtime GitHub notifications bot (Aug 05 2022 at 18:22):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.
-->
Wasmtime GitHub notifications bot (Aug 05 2022 at 18:24):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 theicmpis 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.
-->
Wasmtime GitHub notifications bot (Aug 05 2022 at 18:27):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 05 2022 at 21:52):elliottt updated PR #4625 from trevor/x64-icmp-bool-opt to main.
Wasmtime GitHub notifications bot (Aug 06 2022 at 03:54):MaxGraey submitted PR review.
Wasmtime GitHub notifications bot (Aug 06 2022 at 03:54):MaxGraey created PR review comment:
Hmm, why it uses signed right shift now? It should be an unsigned shift
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:00):MaxGraey submitted PR review.
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:00):MaxGraey created PR review comment:
Is it correct? I guess it should be:
notl %edi, %edi shrl $31, %edi, %edi
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:04):MaxGraey submitted PR review.
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:04):MaxGraey created PR review comment:
Doesn't ISLE and cranelift ir always canonicalize constants on the right?
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:25):MaxGraey edited PR review comment.
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:26):MaxGraey edited PR review comment.
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:26):MaxGraey edited PR review comment.
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:27):MaxGraey edited PR review comment.
Wasmtime GitHub notifications bot (Aug 06 2022 at 04:35):MaxGraey edited PR review comment.
Wasmtime GitHub notifications bot (Aug 08 2022 at 17:56):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Aug 08 2022 at 17:56):elliottt created PR review comment:
This was with an eye towards better supporting our boolean representations, as mentioned in #3205, as
sarwill do sign extension.
Wasmtime GitHub notifications bot (Aug 08 2022 at 17:58):elliottt created PR review comment:
I've inverted it and restricted this optimization to boolean-producing instructions. Also this shouldn't affect the
test/jmpfusion, as this optimization will only fire foricmpinstructions whose results are used directly, not as the input to a branch decision. Was there another fusion case you were thinking this might invalidate?
Wasmtime GitHub notifications bot (Aug 08 2022 at 17:58):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:00):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.
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:00):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:14):MaxGraey submitted PR review.
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:14):MaxGraey created PR review comment:
I meant macro / micro fusion on the CPU side
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:15):MaxGraey deleted PR review comment.
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:15):MaxGraey submitted PR review.
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:15):MaxGraey created PR review comment:
Got it. Thanks!
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:16):MaxGraey submitted PR review.
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:16):MaxGraey created PR review comment:
I see
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:21):elliottt updated PR #4625 from trevor/x64-icmp-bool-opt to main.
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:23):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 toshrwith a type guard on the result. Sorry about the back and forth here!
Wasmtime GitHub notifications bot (Aug 08 2022 at 18:23):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Aug 08 2022 at 19:48):elliottt has marked PR #4625 as ready for review.
Wasmtime GitHub notifications bot (Aug 08 2022 at 19:49):elliottt requested cfallin for a review on PR #4625.
Wasmtime GitHub notifications bot (Aug 09 2022 at 04:20):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 09 2022 at 16:45):elliottt merged PR #4625.
Last updated: Nov 22 2024 at 16:03 UTC