Stream: git-wasmtime

Topic: wasmtime / PR #4599 x64: Migrate brff and I128 branching ...

Wasmtime GitHub notifications bot (Aug 03 2022 at 20:30):

elliottt opened PR #4599 from trevor/x64-isle-br-i128 to main:

• Migrate the implementation of brz and brnz for I128
• Remove unused 8-bit ALU instructions
• Finish migrating branch instructions for floats

<!--

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 03 2022 at 20:31):

elliottt updated PR #4599 from trevor/x64-isle-br-i128 to main.

Wasmtime GitHub notifications bot (Aug 03 2022 at 20:41):

elliottt edited PR #4599 from trevor/x64-isle-br-i128 to main:

I changed the lowering of comparisons for I128 to always end with test instead of and or or depending on whether the context was a brz or brnz instruction. An effect of this change was that the 8-bit variants of the And and Or alu opcodes were no longer needed. The change that this introduces is visible in the i128.clif test output update.

I've also finished the migration of instructions that require floating point comparisons, and removed all the unused code that stemmed from the rust implementation of emit_cmp. This change required adding new constructors to both SideEffectNoResults and ConsumesFlags, as the AndCondition and OrCondition constructors of FcmpCompResult both lower to two jump instructions. This could be re-implemented as two new pseudo-instructions, JmpCondAnd and JmpCondOr, if it seems like this isn't the right approach.

<!--

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 03 2022 at 20:54):

elliottt updated PR #4599 from trevor/x64-isle-br-i128 to main.

Wasmtime GitHub notifications bot (Aug 03 2022 at 20:57):

elliottt edited PR #4599 from trevor/x64-isle-br-i128 to main:

I changed the lowering of comparisons for I128 to always end with test instead of and or or depending on whether the context was a brz or brnz instruction. An effect of this change was that the 8-bit variants of the And and Or alu opcodes were no longer needed. The change that this introduces is visible in the i128.clif test output update.

I've also finished the migration of instructions that require floating point comparisons, and removed all the unused code that stemmed from the rust implementation of emit_fcmp. This change required adding new constructors to both SideEffectNoResults and ConsumesFlags, as the AndCondition and OrCondition constructors of FcmpCompResult both lower to two jump instructions. This could be re-implemented as two new pseudo-instructions, JmpCondAnd and JmpCondOr, if it seems like this isn't the right approach.

<!--

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 03 2022 at 21:06):

elliottt has marked PR #4599 as ready for review.

Wasmtime GitHub notifications bot (Aug 03 2022 at 21:06):

elliottt requested cfallin for a review on PR #4599.

Wasmtime GitHub notifications bot (Aug 03 2022 at 22:17):

elliottt updated PR #4599 from trevor/x64-isle-br-i128 to main.

Wasmtime GitHub notifications bot (Aug 03 2022 at 22:18):

elliottt updated PR #4599 from trevor/x64-isle-br-i128 to main.

Wasmtime GitHub notifications bot (Aug 03 2022 at 22:27):

elliottt updated PR #4599 from trevor/x64-isle-br-i128 to main.

Wasmtime GitHub notifications bot (Aug 03 2022 at 23:08):

elliottt updated PR #4599 from trevor/x64-isle-br-i128 to main.

Wasmtime GitHub notifications bot (Aug 03 2022 at 23:18):

elliottt edited PR #4599 from trevor/x64-isle-br-i128 to main:

I changed the lowering of comparisons for I128 to always end with test instead of and or or depending on whether the context was a brz or brnz instruction. An effect of this change was that the 8-bit variants of the And and Or alu opcodes were no longer needed. The change that this introduces is visible in the i128.clif test output update.

I've also finished the migration of instructions that require floating point comparisons, and removed all the unused code that stemmed from the rust implementation of emit_fcmp. This change required adding new constructors to both SideEffectNoResults and ConsumesFlags, as the AndCondition and OrCondition constructors of FcmpCompResult both lower to two jump instructions. This could be re-implemented as two new pseudo-instructions, JmpCondAnd and JmpCondOr, if it seems like this isn't the right approach.

The test cases added in filetests/isa/x64/branches.clif were generated on the main branch, which helped with debugging the brz (fcmp ...) lowering.

<!--

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 03 2022 at 23:36):

elliottt has marked PR #4599 as ready for review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 00:57):

cfallin created PR review comment:

Can this go away completely now (i.e. constant-prop the false outward to wherever it's called, if anywhere)?

Wasmtime GitHub notifications bot (Aug 04 2022 at 00:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 00:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 00:58):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 00:58):

elliottt created PR review comment:

Definitely! I was a little unsure if removing Add8 and Or8 was desirable, so I didn't push the refactoring as far as I could have :)

Wasmtime GitHub notifications bot (Aug 04 2022 at 01:05):

cfallin created PR review comment:

We likely won't need them for anything else I think -- the actual 8-bit ops use wider opcodes because it's more efficient microarchitecturally (no partial-register stalls) and the upper bits are don't-cares; at least or8 was only needed here because we used SETcc which only sets the lower 8. We can always add them back if we find they're needed!

Wasmtime GitHub notifications bot (Aug 04 2022 at 01:05):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 01:06):

elliottt updated PR #4599 from trevor/x64-isle-br-i128 to main.

Wasmtime GitHub notifications bot (Aug 04 2022 at 15:58):

elliottt merged PR #4599.

Last updated: Dec 23 2024 at 12:05 UTC