elliottt opened PR #6253 from elliottt:trevor/s390x-avoid-copy
to bytecodealliance:main
:
After the switch to providing SSA input to RA2, these uses of
copy_reg
should no longer be necessary. Additionally, as RA2-0.7.0 has removed support for program moves, these moves will no longer be optimized away.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt has marked PR #6253 as ready for review.
elliottt requested jameysharp for a review on PR #6253.
elliottt requested wasmtime-compiler-reviewers for a review on PR #6253.
elliottt edited PR #6253:
After the switch to providing SSA input to RA2, these uses of
copy_reg
should no longer be necessary. Additionally, as RA2-0.7.0 has removed support for program moves, these moves will no longer be optimized away.@uweigand, could you take a look as well? These uses of
copy_reg
all looked like they'd be fine to remove, but I could be missing something.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
jameysharp submitted PR review:
This seems reasonable to me, and Ulrich okayed it, so I'm happy.
I would like to understand the two cases where this adds an extra move though, ideally before merging.
jameysharp submitted PR review:
This seems reasonable to me, and Ulrich okayed it, so I'm happy.
I would like to understand the two cases where this adds an extra move though, ideally before merging.
jameysharp created PR review comment:
I'm curious about the extra move here too.
jameysharp created PR review comment:
Any idea why this is inserting an extra move?
elliottt created PR review comment:
The moves not existing in the input removes a degree of freedom for RA2, and as a result the tight constraints imposed from the abi end up forcing a move in the middle of the program. If we instead compile this function:
function %srem_i16(i16, i16) -> i16 { block0(v0: i16, v1: i16): v2 = iadd v1, v1 v3 = srem.i16 v0, v2 return v3 }
The output looks much better, as the indirection of
v1
through a register other thanr3
allows for better allocation:0: b9 f8 30 53 ark %r5, %r3, %r3 4: b9 07 00 32 lghr %r3, %r2 8: b9 27 00 45 lhr %r4, %r5 c: b9 1d 00 24 dsgfr %r2, %r4 10: 07 fe br %r14 ``` ~~~
elliottt created PR review comment:
I think removing the copies had the effect of shuffling the order that the registers are picked, which caused the allocator to find a case where it needed to insert a move. Additionally, adding the move on
res_0
back caused the second move to disappear again (even on the RA2-0.7.0 branch), so I'm going to put that one back and call this good.
elliottt updated PR #6253.
elliottt has enabled auto merge for PR #6253.
elliottt merged PR #6253.
Last updated: Dec 23 2024 at 12:05 UTC