Stream: git-wasmtime

Topic: wasmtime / PR #6253 s390x: Remove uses of copy_reg

Wasmtime GitHub notifications bot (Apr 20 2023 at 17:29):

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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 20 2023 at 18:09):

elliottt has marked PR #6253 as ready for review.

Wasmtime GitHub notifications bot (Apr 20 2023 at 18:09):

elliottt requested jameysharp for a review on PR #6253.

Wasmtime GitHub notifications bot (Apr 20 2023 at 18:09):

elliottt requested wasmtime-compiler-reviewers for a review on PR #6253.

Wasmtime GitHub notifications bot (Apr 20 2023 at 18:09):

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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 20 2023 at 19:04):

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.

Wasmtime GitHub notifications bot (Apr 20 2023 at 19:04):

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.

Wasmtime GitHub notifications bot (Apr 20 2023 at 19:04):

jameysharp created PR review comment:

I'm curious about the extra move here too.

Wasmtime GitHub notifications bot (Apr 20 2023 at 19:04):

jameysharp created PR review comment:

Any idea why this is inserting an extra move?

Wasmtime GitHub notifications bot (Apr 20 2023 at 20:20):

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 than r3 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
  ```
~~~

Wasmtime GitHub notifications bot (Apr 20 2023 at 21:35):

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.

Wasmtime GitHub notifications bot (Apr 20 2023 at 21:37):

elliottt updated PR #6253.

Wasmtime GitHub notifications bot (Apr 20 2023 at 21:41):

elliottt has enabled auto merge for PR #6253.

Wasmtime GitHub notifications bot (Apr 20 2023 at 22:45):

elliottt merged PR #6253.

Last updated: Nov 22 2024 at 16:03 UTC