Stream: git-wasmtime

Topic: wasmtime / PR #4598 [s390x, abi_impl] Add i128 support

Wasmtime GitHub notifications bot (Aug 03 2022 at 19:21):

uweigand opened PR #4598 from s390x-i128 to main:

This adds full i128 support to the s390x target, including new filetests
and enabling the existing i128 runtest on s390x.

The ABI requires that i128 is passed and returned via implicit pointer,
but the front end still generates direct i128 types in call. This means
we have to implement ABI support to implicitly convert i128 types to
pointers when passing arguments.

To do so, we add a new variant ABIArg::ImplicitArg. This acts like
StructArg, except that the value type is the actual target type,
not a pointer type. The required conversions have to be inserted
in the prologue and at function call sites.

Note that when dereferencing the implicit pointer in the prologue,
we may require a temp register: the pointer may be passed on the
stack so it needs to be loaded first, but the value register may
be in the wrong class for pointer values. In this case, we use
the "stack limit" register, which should be available at this
point in the prologue.

For return values, we use a mechanism similar to the one used for
supporting multiple return values in the Wasmtime ABI. The only
difference is that the hidden pointer to the return buffer must
be the first, not last, argument in this case.

(FYI @cfallin - This implements the second half of issue #4565.)

<!--

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 (Aug 04 2022 at 17:11):

cfallin created PR review comment:

As an alternative I suppose we could provide an "allocate temp vreg" callback to gen_copy_arg_to_regs; this codegen happens before regalloc so it is still an option to create vregs here. Perhaps a comment here to record the altnernate possibility? (We may in the future want to remove the need for more dedicated non-allocatable registers, at which point it may become more relevant.)

Wasmtime GitHub notifications bot (Aug 04 2022 at 17:11):

cfallin created PR review comment:

Can we call this an ImplicitPtrArg or similar? Just ImplicitArg seems slightly ambiguous to me; like the arg itself is implicitly generated at callsites or something like that.

Wasmtime GitHub notifications bot (Aug 04 2022 at 17:11):

cfallin created PR review comment:

Comment somewhere in here that we write tmp only after the last reads of rn and rm, as required by the semantics of ordinary (early) uses and ordinary (late) defs? And perhaps a note that if this changes, tmp has to become an early def. (I mostly want to future-proof against a subtle mistake later.)

Wasmtime GitHub notifications bot (Aug 04 2022 at 17:11):

cfallin created PR review comment:

Add a note here that s390x is not yet supported because (reason)?

Wasmtime GitHub notifications bot (Aug 04 2022 at 17:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 17:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 19:55):

uweigand updated PR #4598 from s390x-i128 to main.

Wasmtime GitHub notifications bot (Aug 04 2022 at 19:56):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 19:56):

uweigand created PR review comment:

Done

Wasmtime GitHub notifications bot (Aug 04 2022 at 19:56):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 19:56):

uweigand created PR review comment:

Done

Wasmtime GitHub notifications bot (Aug 04 2022 at 20:00):

uweigand created PR review comment:

Ah yes, that's better. However, I wasn't able to get a callback past the borrow checker - maybe I'm just not experienced enough with Rust, but it does look a real problem:

                for insn in self.vcode.abi().gen_copy_arg_to_regs(i, regs).into_iter() {

This already holds a borrow on self during the gen_copy_arg_to_regs call. Passing a closure would require another (mutable) borrow on self, which isn't allowed.

Instead, I noticed that there is already a mechanism to pass a temp reg to ABICallee via the temp_needed/init mechanism, so I simply extended that to support multiple temps.

Wasmtime GitHub notifications bot (Aug 04 2022 at 20:00):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 20:00):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 20:00):

uweigand created PR review comment:

Done

Wasmtime GitHub notifications bot (Aug 04 2022 at 20:16):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2022 at 20:16):

cfallin has enabled auto merge for PR #4598.

Wasmtime GitHub notifications bot (Aug 04 2022 at 20:41):

cfallin merged PR #4598.

Last updated: Oct 23 2024 at 20:03 UTC