Stream: git-wasmtime

Topic: wasmtime / PR #5172 Generate SSA code from returns

Wasmtime GitHub notifications bot (Nov 01 2022 at 23:01):

elliottt opened PR #5172 from trevor/ret-ssa to main:

Modify return pseudoinstructions to have pairs of registers: virtual and real. This allows us to constrain the virtual registers to the real ones specified by the abi, instead of directly emitting moves to those real regisers.

Additionally, this PR marks r14 on s390x as non-allocatable, as it's used as the link register in the return instruction.
<!--

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 (Nov 01 2022 at 23:47):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 02 2022 at 17:12):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 17:12):

cfallin created PR review comment:

This is going to make things a little interesting too: properly speaking, an extend should allocate a new temp for fully SSA output, rather than coerce the preg to a Writable and extend into it.

Wasmtime GitHub notifications bot (Nov 02 2022 at 17:12):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:21):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 02 2022 at 20:49):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 03 2022 at 18:13):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 03 2022 at 18:18):

elliottt has marked PR #5172 as ready for review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 19:24):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 19:24):

elliottt created PR review comment:

Neat, we avoided a move :tada:

Wasmtime GitHub notifications bot (Nov 03 2022 at 20:54):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:01):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:12):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:12):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:12):

uweigand created PR review comment:

Hi @elliottt this seems a bug: the lgr overwrites the effect of the llgfr. Is there some problem with the extension handling on return values?

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:12):

uweigand created PR review comment:

Hi @elliottt this seems a bug: the lgr overwrites the effect of the llgfr. Is there some problem with the extension handling on return values?

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:38):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:38):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 21:38):

uweigand created PR review comment:

Doesn't this now violate the x64 ABI? I thought this requires the struct-ret pointer to be returned in %rax, that was the whole point of the logic of adding a StructReturn element to the return values?

Wasmtime GitHub notifications bot (Nov 03 2022 at 23:13):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 03 2022 at 23:53):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 23:53):

elliottt created PR review comment:

There are currently some problems with struct-return that I'm tracking down. I think all the comments you added are directly related.

Wasmtime GitHub notifications bot (Nov 04 2022 at 00:17):

elliottt created PR review comment:

@uweigand: I think that Chris's comment here is the same problem you noticed with extension on returns.

Wasmtime GitHub notifications bot (Nov 04 2022 at 00:17):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2022 at 21:28):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 04 2022 at 21:29):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2022 at 21:29):

elliottt created PR review comment:

@uweigand could you take another look? After the change to introduce a temporary for the extension, these tests also lack the initial lgr instruction, turning into just llgfr %r2, %r2.

Wasmtime GitHub notifications bot (Nov 04 2022 at 21:48):

uweigand created PR review comment:

That's even better, removes an unnecessary copy and just zero-extends %r2 in place. The s390x test changes look all good to me now. (Mostly just renamed registers; in several cases we get better code by eliminating register copies or spills, and a few cases -notably relating to divide instructions- we get slightly worse code. Overall still good though.)

Wasmtime GitHub notifications bot (Nov 04 2022 at 21:48):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Nov 04 2022 at 23:00):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 07 2022 at 04:44):

elliottt edited PR #5172 from trevor/ret-ssa to main:

Modify return pseudoinstructions to have pairs of registers: virtual and real. This allows us to constrain the virtual registers to the real ones specified by the abi, instead of directly emitting moves to those real regisers.
<!--

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 (Nov 07 2022 at 20:30):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 08 2022 at 17:52):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 08 2022 at 18:07):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 08 2022 at 18:42):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 08 2022 at 18:43):

elliottt created PR review comment:

This should be fixed now.

Wasmtime GitHub notifications bot (Nov 08 2022 at 18:43):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 08 2022 at 18:45):

elliottt requested cfallin for a review on PR #5172.

Wasmtime GitHub notifications bot (Nov 08 2022 at 18:56):

elliottt updated PR #5172 from trevor/ret-ssa to main.

Wasmtime GitHub notifications bot (Nov 08 2022 at 21:39):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 09 2022 at 00:00):

elliottt merged PR #5172.

Last updated: Nov 22 2024 at 16:03 UTC