Stream: git-wasmtime

Topic: wasmtime / PR #2346 machinst ABI: Pass fixed frame size t...

Wasmtime GitHub notifications bot (Nov 02 2020 at 12:23):

uweigand opened PR #2346 from abi-noframepointer to main:

The ABI common code currently passes the fixed frame size to
the gen_clobber_save back-end routine, which is required to
emit code to allocate the required stack space in the prologue.

Similarly, the back-end needs to emit code to de-allocate the
stack in the epilogue. However, at this point the back-end
does not have access to that fixed frame size value any more.
With targets that use a frame pointer, this does not matter,
since de-allocation can be done simply by assigning the frame
pointer back to the stack pointer. However, on targets that
do not use a frame pointer, the frame size is required.

To allow back-ends that option, this patch changes ABI common
code to pass the fixed frame size to get_clobber_restore as
well (computed identically to how it is done for get_clobber_save).

<!--

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 02 2020 at 12:26):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 12:26):

bjorn3 created PR Review Comment:

        let fixed_frame_storage_size = if !self.call_conv.extends_baldrdash() {
            self.frame_size()
        } else {
            0
        };

Also why this condition?

Wasmtime GitHub notifications bot (Nov 02 2020 at 12:35):

uweigand submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 12:35):

uweigand created PR Review Comment:

If you look at the code in gen_prologue above, it passes a fixed_frame_storage_size of 0 to gen_clobber_save with the baldrdash calling convention. For consistency, we need to do the same for gen_clobber_restore.

Likewise, I followed the same coding convention with the "mut fixed_frame_storage_size" as is currently in place in gen_prologue. Given that any future changes to the computation of the size will need to be kept in sync between gen_prologue and gen_epilogue, it seems preferable to me to also have the code itself as similar as possible.

Wasmtime GitHub notifications bot (Nov 02 2020 at 19:08):

cfallin created PR Review Comment:

I think it would be better to simply store the fixed_frame_storage_size in self -- we get a mutable self in gen_prologue so we should be able to write it there. This avoids the duplication of logic and potential for mismatches...

Wasmtime GitHub notifications bot (Nov 02 2020 at 19:08):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 19:08):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 10:23):

uweigand updated PR #2346 from abi-noframepointer to main:

The ABI common code currently passes the fixed frame size to
the gen_clobber_save back-end routine, which is required to
emit code to allocate the required stack space in the prologue.

Similarly, the back-end needs to emit code to de-allocate the
stack in the epilogue. However, at this point the back-end
does not have access to that fixed frame size value any more.
With targets that use a frame pointer, this does not matter,
since de-allocation can be done simply by assigning the frame
pointer back to the stack pointer. However, on targets that
do not use a frame pointer, the frame size is required.

To allow back-ends that option, this patch changes ABI common
code to pass the fixed frame size to get_clobber_restore as
well (computed identically to how it is done for get_clobber_save).

<!--

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 03 2020 at 10:23):

uweigand submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 10:23):

uweigand created PR Review Comment:

OK, updated the patch accordingly.

Wasmtime GitHub notifications bot (Nov 03 2020 at 17:00):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 17:01):

cfallin merged PR #2346.

Last updated: Nov 22 2024 at 16:03 UTC