Stream: git-wasmtime

Topic: wasmtime / PR #4573 Cranelift: Add instructions for getti...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:23):

fitzgen edited PR #4573 from clif-stack-frame-return-pointer to main:

This is the initial part of https://github.com/bytecodealliance/wasmtime/issues/4535

TODO: I need to implement the s390x lowering for these new instructions.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:36):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:36):

cfallin created PR review comment:

I would instead assert that the register is virtual; we should never have a preg-to-preg move inserted as that would either have undefined behavior (if allocatable register) or would break the ABI (move to SP or FP).

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:36):

cfallin created PR review comment:

Can we assert that rm is one of a few registers on an allow-list?

Ideally the property we want here is that it is not an allocatable register. Moving from a register managed by the regalloc has undefined behavior: we don't know what the value will be. But grabbing the MachineEnv and doing an assert on the lists of registers there is sort of cumbersome, so it's probably better if we limit it first to e.g. FP and SP (with a comment describing the real condition) and then add more regs to the list if we need them.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:36):

cfallin created PR review comment:

Rather than duplicate the mov code, could we do Inst::Mov { .. }.emit(...), in the same way other pseudoinsts work?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:36):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:36):

cfallin created PR review comment:

a tiny bikeshed-color nit, but I think usually this intrinsic refers to the "return address" rather than "return pointer" (e.g., __builtin_return_address() in GCC/Clang); perhaps we could call it that?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:36):

cfallin created PR review comment:

Ah! I see you had the same thought I did, reading a bit further :-)

I think the answer is definitely yes!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 23:36):

cfallin created PR review comment:

I suspect we could avoid the extra copy here by defining the return-address intrinsic to load directly from 8(%rbp), i.e. creating an Amode with rbp as a base register.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 00:40):

fitzgen updated PR #4573 from clif-stack-frame-return-pointer to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 01:19):

fitzgen updated PR #4573 from clif-stack-frame-return-pointer to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 01:19):

fitzgen updated PR #4573 from clif-stack-frame-return-pointer to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 01:20):

fitzgen has marked PR #4573 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 01:20):

fitzgen requested cfallin for a review on PR #4573.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 01:24):

fitzgen edited PR #4573 from clif-stack-frame-return-pointer to main:

This is the initial part of https://github.com/bytecodealliance/wasmtime/issues/4535

TODO: I need to implement the s390x lowering for these new instructions.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 01:38):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 01:38):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 01:38):

cfallin created PR review comment:

This is to avoid providing the base reg as an operand to the operand collector, I guess?

I wonder if it might be cleaner, or less error-prone at least, to handle this case in the Amode::get_operands and Amode::with_allocs methods instead: skip providing rbp (or rsp) as an operand, and skip pulling an alloc from the AllocationConsumer if the original register was rbp (or rsp).

(I'm actually not sure; I'm going back and forth right now.)

The general concern I had with this is that we may not handle the same lowering everywhere. But actually I think everything funnels to this one lowering helper so maybe this is sufficient.

If we do stick to this approach, let's add a comment where we define the RbpOffset arm that it must be used for rbp (as opposed to the generic forms) because rbp cannot be given as an arg to regalloc.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 16:45):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 16:45):

fitzgen created PR review comment:

Yeah I actually like this better.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:07):

fitzgen updated PR #4573 from clif-stack-frame-return-pointer to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:41):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:41):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:41):

cfallin created PR review comment:

I think we need to mirror this conditional over to with_allocs here as well; otherwise we have a thing that consumes more allocs than the operands it reports. This will be kind-of-harmless today because Amode's operands are always placed at the end of the inst by convention (because they're variable-length and that was easier wrt reuse-indices) but if a pseudoinst ever grows two Amodes or puts something after an Amode that could be an issue.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:41):

cfallin created PR review comment:

We should assert here that base and index are not rbp or rsp I think, now that we're admitting we may use these registers in an Amode, just for consistency...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:43):

uweigand created PR review comment:

Just a suggestion to use something like preg_stack instead of r15, since this is actually not an ISA requirement - other ABIs use different registers as stack pointer.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:43):

uweigand submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:43):

uweigand created PR review comment:

I'd remove gpr(14), since this is actually allocatable. And again preferably use stack_reg instead of hardcoding 15.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:43):

uweigand submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 20:13):

fitzgen updated PR #4573 from clif-stack-frame-return-pointer to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 21:37):

fitzgen merged PR #4573.

Last updated: Nov 22 2024 at 16:03 UTC