Stream: git-wasmtime

Topic: wasmtime / PR #3724 s390x: Migrate branches and traps to ...

Wasmtime GitHub notifications bot (Jan 25 2022 at 17:36):

uweigand opened PR #3724 from s390x-isle-branchtrap to main:

In order to migrate branches to ISLE, we define a second entry
point lower_branch which gets the list of branch targets as
additional argument.

This requires a small change to lower_common: the isle_lower
callback argument is changed from a function pointer to a closure.
This allows passing the extra argument via a closure.

Traps make use of the recently added facility to emit safepoints
from ISLE, but are otherwise straightforward.

@cfallin @fitzgen

<!--

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 (Jan 25 2022 at 18:13):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:13):

fitzgen created PR review comment:

I think this should go in the shared prelude, next to value_regs_none.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:13):

fitzgen created PR review comment:

And then I would expect this to go into wasmtime/cranelift/codegen/src/machinst/isle.rs

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:13):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:13):

fitzgen created PR review comment:

And this should be in the prelude as well.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:14):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:14):

fitzgen created PR review comment:

I see that emit actually isn't in the prelude, but we do use it from the prelude. So we should probably move emit to the prelude as well, since in practice it has to be the same declaration across all backends anyways. Happy to have that as part of this updated PR or we can do it in another PR.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:19):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:19):

uweigand created PR review comment:

Yes, that's why I didn't put it in the prelude ...

I agree it makes sense to move emit / emit_safepoint there, but then we should also move the implementations to machinst/isle.re, right?

I'd be happy to do that, but I think it would be better to do it in another PR, as it will touch all back-ends.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:40):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:40):

fitzgen created PR review comment:

Yes, we wouldn't necessarily have to move the implementations as well, nothing is stopping us from implementing the ISLE context's emit trait method once for each backend even when the decl is in the prelude.

In fact the x86_64 implementation is a little different, in that it calls a mov_mitosis method on each inst in emitted_insts, and if we made this code common then we would need to add such a method to the MachInst trait or something.

So I think we can move just the ISLE decl into the prelude without moving the rust trait implementations at all. Happy to have this in a follow up PR.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:45):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:45):

uweigand created PR review comment:

Ah, I see. But even if we don't move the implementaton, once the decl is in the prelude as extern constructor, every back-end must implement it or it won't compile any more. So we'll still need to touch all back-ends. (In fact, with the x86 mov_mitosis implementation, I'm not sure exactly how the safepoint variant would work. I guess only the last instruction in the list should be the safepoint?)

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:48):

fitzgen created PR review comment:

once the decl is in the prelude as extern constructor, _every_ back-end _must_ implement it or it won't compile any more.

Correct. (Also, it is already the case that every backend must implement it or the ISLE won't compile because we are calling emit and assuming it is declared from inside the prelude already, it just so happens that we have N identical declarations for each backend instead of a single shared declaration in the prelude.)

(In fact, with the x86 mov_mitosis implementation, I'm not sure exactly how the safepoint variant would work. I guess only the last instruction in the list should be the safepoint?)

Yeah, I will have to think a bit about how to do this best. I think making only the last instruction a safepoint would work, but we might want to have some debug asserts in there too. Planning on tackling this after I'm done with my newtype wrappers PR by which time this PR will have merged.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:48):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:56):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:56):

cfallin created PR review comment:

I think making only the last instruction a safepoint would work,

Yeah, I think that's the correct solution -- the expanded list should always have the form of zero or more moves, followed by the original instruction; and the safepoint bit applies to the latter.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:59):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2022 at 18:59):

fitzgen created PR review comment:

There are some instructions that always generate their results into a particular register, and so when our SSA-ified version of that instruction wants the result in a temp, we have to generate a move that follows the "real" instruction, so we can have trailing moves as well after move mitosis. This shouldn't be the case for safepoints I don't think (except maybe ABI/calling convention stuff where results are always in rax?) and we should be able to just debug assert this.

Wasmtime GitHub notifications bot (Jan 31 2022 at 21:42):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2022 at 21:42):

fitzgen merged PR #3724.

Last updated: Nov 22 2024 at 16:03 UTC