Stream: git-wasmtime

Topic: wasmtime / issue #4573 Cranelift: Add instructions for ge...

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

github-actions[bot] commented on issue #4573:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "isle"

Thus the following users have been cc'd because of the following labels:

• cfallin: isle
• fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 02 2022 at 01:23):

fitzgen commented on issue #4573:

@uweigand would you mind taking a look at the s390x-specific bits in here? I'm not 100% sure I got it right. I think you should be able to just look at the test expectations to quickly check here. Thanks!

Wasmtime GitHub notifications bot (Aug 02 2022 at 06:46):

bjorn3 commented on issue #4573:

How are these instructions expected to behave in the face of inlining?

Wasmtime GitHub notifications bot (Aug 02 2022 at 12:31):

uweigand commented on issue #4573:

@uweigand would you mind taking a look at the s390x-specific bits in here? I'm not 100% sure I got it right. I think you should be able to just look at the test expectations to quickly check here. Thanks!

Hi @fitzgen, the get_return_address implementation isn't correct in the general case. It looks like you're assuming r14 holds the return address throughout execution of the function? That isn't true, it is only guaranteed to hold the return address at the ABI boundaries, everywhere else it's just a normal allocatable general-purpose register.

For this is to be always correct, we instead have to read the return address from the stack slot where it was saved in the prologue. However, that doesn't happen in leaf functions - unless it is forced via the preserve_frame_pointers flag. Can we make that assumption for get_return_address? It seems it is already made for get_frame_pointer.

If so, we can read the return address from the slot at "initial stack pointer + 14*8". This could be done either via MemArg::InitialSPOffset (which currently isn't exposed to ISLE yet, but it should be straightforward to add a memarg_initial_stack_off or similar). In the alternative, the initial stack pointer actually equals the frame pointer (backchain) value, so it could also be implemented via something like (memarg_reg_plus_off (get_frame_pointer) 112 0 (memarg_trusted)).

Also, I'm a bit confused about why that new mov_preg thing is necessary. I'd have expected to use something like (copy_reg (writable_reg 15)) instead, or maybe even better some new (copy_reg (stack_reg)) - this is already used in other places to retrieve the value of a physical register.

(Or, in the alternative, even something like (load_addr (memarg_stack_off 0)) would work to get the stack pointer.)

Wasmtime GitHub notifications bot (Aug 02 2022 at 16:44):

fitzgen commented on issue #4573:

How are these instructions expected to behave in the face of inlining?

We don't have inlining so it is a bit of a hypothetical question at the moment, but these instructions are about physical frames, so I imagine we would want to ignore logical frames that don't have a corresponding physical frame (i.e. they have been inlined) and just return information about the current physical frame regardless.

Wasmtime GitHub notifications bot (Aug 02 2022 at 16:46):

fitzgen commented on issue #4573:

Also, I'm a bit confused about why that new mov_preg thing is necessary.

It is about getting an unallocatable physical register's value into a virtual register, and avoiding the pinned vregs that we want to remove support for from regalloc2, and which just existed in order to get the initial migration working.

Wasmtime GitHub notifications bot (Aug 02 2022 at 16:49):

cfallin commented on issue #4573:

Also, I'm a bit confused about why that new mov_preg thing is necessary. I'd have expected to use something like (copy_reg (writable_reg 15)) instead, or maybe even better some new (copy_reg (stack_reg)) - this is already used in other places to retrieve the value of a physical register.

This came out of some discussion that @fitzgen and I had: I would like to push the backends gradually away from using the "pinned vregs", or vregs that represent physical registers directly, and instead use operand constraints wherever possible. This is because handling moves-from-pinned and moves-to-pinned-vregs introduces a bunch of special cases in regalloc, and leads to worse performance (it uses a whole instruction, the move, to communicate what should be just a constraint) and sometimes bugs (e.g. bytecodealliance/regalloc2#60). Just yesterday I was helping to find the cause for a weird mov rA, rB; mov rB, rA sequence and it turned out to be because of moves-to-pregs before a call (in x64 in this case) where operand constraints would have led to properly elided moves.

So it's a bit forward-looking, but there is a reason! At some point later I (or someone) will go through and systematically update the remaining uses.

Wasmtime GitHub notifications bot (Aug 02 2022 at 17:09):

fitzgen commented on issue #4573:

Okay I pushed a couple new commits:

• switched x64 away from the new variant of Amode and just check the base register when getting operands for Amode::ImmReg, which I think is a bit nicer.
• Fixed s390x per @uweigand's feedback (thanks! would appreciate if you could give this another quick once over)

Wasmtime GitHub notifications bot (Aug 02 2022 at 17:44):

uweigand commented on issue #4573:

Okay I pushed a couple new commits:
* Fixed s390x per @uweigand's feedback (thanks! would appreciate if you could give this another quick once over)

LGTM. I've added a couple of inline comments, but those are really just cosmetic.

Wasmtime GitHub notifications bot (Aug 02 2022 at 17:47):

uweigand commented on issue #4573:

Okay I pushed a couple new commits: * Fixed s390x per @uweigand's feedback (thanks! would appreciate if you could give this another quick once over)

LGTM. I've added a couple of inline comments, but those are really just cosmetic.

Oh, maybe one more thing comes to mind: it would be great if the affected ISLE patterns actually verified that preserve_frame_pointer is true if they depend on that fact. Maybe some predicate in prelude.isle that could be used like so:

(rule (lower (get_frame_pointer))
      (if (preserve_frame_pointer))
      (load64 (memarg_stack_off 0 0)))

Then we'd get a compile-time error instead of random run-time crashes if the assumption was violated.

Wasmtime GitHub notifications bot (Aug 02 2022 at 17:48):

uweigand commented on issue #4573:

I would like to push the backends gradually away from using the "pinned vregs", or vregs that represent physical registers directly, and instead use operand constraints wherever possible.

I see, thanks for the explanation!

Wasmtime GitHub notifications bot (Aug 02 2022 at 20:11):

fitzgen commented on issue #4573:

Oh, maybe one more thing comes to mind: it would be great if the affected ISLE patterns actually _verified_ that preserve_frame_pointer is true if they depend on that fact.

The CLIF verifier does check this condition, so by the time we lower the CLIF, we know we have valid CLIF, and this check should be unnecessary. I wouldn't mind the redundancy if it were a simple one-line debug assertion, but since it would involve new external ISLE extractors, I don't think it is quite worth it.

Wasmtime GitHub notifications bot (Aug 02 2022 at 22:35):

uweigand commented on issue #4573:

Oh, maybe one more thing comes to mind: it would be great if the affected ISLE patterns actually _verified_ that preserve_frame_pointer is true if they depend on that fact.

The CLIF verifier does check this condition

Ah, good! I hadn't see that.

Wasmtime GitHub notifications bot (Aug 03 2022 at 18:20):

bjorn3 commented on issue #4573:

I didn't see an answer to my question in https://github.com/bytecodealliance/wasmtime/pull/4573#issuecomment-1202084202.

Wasmtime GitHub notifications bot (Aug 03 2022 at 18:40):

cfallin commented on issue #4573:

I didn't see an answer to my question in #4573 (comment).

@fitzgen replied to your comment in this comment.

Wasmtime GitHub notifications bot (Aug 04 2022 at 12:51):

bjorn3 commented on issue #4573:

Missed that comment. Thanks @cfallin.

Last updated: Dec 23 2024 at 12:05 UTC