Stream: git-wasmtime

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


view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 12:26):

bjorn3 submitted PR Review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 12:35):

uweigand submitted PR Review.

view this post on Zulip 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.

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 19:08):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2020 at 19:08):

cfallin submitted PR Review.

view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 10:23):

uweigand submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 10:23):

uweigand created PR Review Comment:

OK, updated the patch accordingly.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 17:00):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 17:01):

cfallin merged PR #2346.


Last updated: Dec 23 2024 at 12:05 UTC