Stream: git-wasmtime

Topic: wasmtime / PR #5440 Assert that we only use virtual regis...

Wasmtime GitHub notifications bot (Dec 14 2022 at 21:09):

elliottt opened PR #5440 from trevor/assert-virtual-regs-move to main:

<!--

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:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:21):

elliottt updated PR #5440 from trevor/assert-virtual-regs-move to main.

Wasmtime GitHub notifications bot (Dec 20 2022 at 19:50):

elliottt updated PR #5440 from trevor/assert-virtual-regs-move to main.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:15):

elliottt requested cfallin for a review on PR #5440.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:17):

elliottt updated PR #5440 from trevor/assert-virtual-regs-move to main.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:18):

elliottt updated PR #5440 from trevor/assert-virtual-regs-move to main.

Wasmtime GitHub notifications bot (Dec 20 2022 at 20:19):

elliottt has marked PR #5440 as ready for review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 22:29):

elliottt edited PR #5440 from trevor/assert-virtual-regs-move to main:

Assert that we never see real registers as arguments to move instructions in VCodeBuilder::collect_operands.

Also fix a bug in the riscv64 backend that was discovered by these assertions: the lowerings of get_stack_pointer and get_frame_pointer were using physical registers 8 and 2 directly. The solution was similar to other backends: add a move instruction specifically for moving out of physical registers, whose source operand is opaque to regalloc2.
<!--

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 20 2022 at 23:04):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:04):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:04):

cfallin created PR review comment:

Can we add a note here that the PReg must be a non-allocatable register?

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:04):

cfallin created PR review comment:

This generic constructor seems a little dangerous because it allows construction of PReg values for allocatable registers, which should not be usable; these will ideally be caught by an assertion (e.g. when handling moves) but it would be better to avoid constructing them in the first place. Could we expose specific constructors for the registers we want (e.g., fp_reg, sp_reg) here instead?

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:04):

cfallin created PR review comment:

Inadvertent insertion/repetition?

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:04):

cfallin created PR review comment:

Is fmv.d the right mnemonic here? That's a floating-point move, no?

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:09):

elliottt created PR review comment:

Yep, thanks!

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:09):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:13):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:13):

elliottt created PR review comment:

Good catch, I misread the F64 in the above case for Mov.

Wasmtime GitHub notifications bot (Dec 20 2022 at 23:19):

elliottt updated PR #5440 from trevor/assert-virtual-regs-move to main.

Wasmtime GitHub notifications bot (Dec 21 2022 at 02:22):

elliottt merged PR #5440.

Last updated: Dec 23 2024 at 12:05 UTC