Wasmtime GitHub notifications bot (Dec 14 2022 at 01:51): Wasmtime GitHub notifications bot (Dec 14 2022 at 01:51):elliottt edited PR #5430 from trevor/fixed-nonallocatable-aliases to main:
The only situation where we use physical registers now is for fixed non-allocatable registers, all other uses should be virtual register. As such, it no longer makes sense to report moves of registers that aren't virtual to regalloc2, as that would imply that it's acceptable to satisfy a constraint by using a non-allocatable register.
Fixes #5425
<!--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 (Dec 14 2022 at 01:55):elliottt edited PR #5430 from trevor/fixed-nonallocatable-aliases to main:
The
casloop_emitfunction in the s390x backend was using the fixed non-allocatable register%r0directly with move instructions, which produced a panic in the regalloc2 checker (#5425). This PR changes thecasloop_resultfunction to usemov_preginstead ofcopy_regto fetch the result, as it's not viewed by regalloc2 as a move. However, this does introduce some indirection when we need to swap the endianness of the result, as we will first move out and then swap.Fixes #5425
<!--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 (Dec 14 2022 at 03:27):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Dec 14 2022 at 12:50):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Dec 14 2022 at 12:50):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Dec 14 2022 at 12:50):uweigand created PR review comment:
This a bit iffy, in that it's not guaranteed (at least, that wasn't the intention) that
resultmust be GPR 0 here. I think in practice it (nearly?) always is, but it still would be better to at least verify.
Wasmtime GitHub notifications bot (Dec 14 2022 at 12:50):uweigand created PR review comment:
I'm confused about the exact rules when/which instructions are allowed to refer to non-allocatable fixed registers. It seems the load before loop (loading into %r0) is OK, but the move is problematic (which I understand is because regalloc handles moves specially).
But is it OK to have a fixed register as source of the bswap instruction? If so, we wouldn't have to incur this performance regression. In fact, if only moves are the problem, maybe the fix could be as simple as having a special-cased rule for
copy_regthat detects that the source is a fixed register and uses mov_preg instead?
Wasmtime GitHub notifications bot (Dec 14 2022 at 16:27):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Dec 14 2022 at 16:27):elliottt created PR review comment:
That's a good point: moves are indeed the case that we need to avoid, and it should be fine for the bswap instruction to take a fixed non-allocatable input. I'll fix this up, thanks!
Wasmtime GitHub notifications bot (Dec 14 2022 at 16:28):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Dec 14 2022 at 16:28):elliottt created PR review comment:
I'll verify that it's always used with gpr 0, and add a comment explaining why we return it there if that's the case.
Wasmtime GitHub notifications bot (Dec 14 2022 at 17:38):elliottt updated PR #5430 from trevor/fixed-nonallocatable-aliases to main.
Wasmtime GitHub notifications bot (Dec 14 2022 at 17:51):elliottt updated PR #5430 from trevor/fixed-nonallocatable-aliases to main.
Wasmtime GitHub notifications bot (Dec 14 2022 at 18:43):elliottt updated PR #5430 from trevor/fixed-nonallocatable-aliases to main.
Wasmtime GitHub notifications bot (Dec 14 2022 at 18:44):elliottt updated PR #5430 from trevor/fixed-nonallocatable-aliases to main.
Wasmtime GitHub notifications bot (Dec 14 2022 at 21:06):elliottt edited PR #5430 from trevor/fixed-nonallocatable-aliases to main:
The
casloop_emitfunction in the s390x backend was using the fixed non-allocatable register%r0directly with move instructions, which produced a panic in the regalloc2 checker (#5425). This PR changes thecasloop_resultfunction to usemov_preginstead ofcopy_regto fetch the result, as it's not viewed by regalloc2 as a move.Fixes #5425
<!--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 (Dec 14 2022 at 21:06):elliottt merged PR #5430.
Last updated: Dec 23 2024 at 13:07 UTC