s390x: Migrate branches and traps to ISLE by uweigand · Pull Request #3724 · bytecodealliance/wasmtime

uweigand · 2022-01-25T17:35:59Z

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

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.

fitzgen

Looks great modulo a couple nitpicks below, thanks!

fitzgen · 2022-01-25T18:09:16Z

cranelift/codegen/src/isa/s390x/inst.isle

+(decl safepoint (SideEffectNoResult) ValueRegs)
+(rule (safepoint (SideEffectNoResult.Inst inst))
+      (let ((_ Unit (emit_safepoint inst)))
+        (value_regs_invalid)))


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

fitzgen · 2022-01-25T18:11:30Z

cranelift/codegen/src/isa/s390x/lower/isle.rs

+    #[inline]
+    fn emit_safepoint(&mut self, inst: &MInst) -> Unit {
+        self.emitted_insts.push((inst.clone(), true));
+    }


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

fitzgen · 2022-01-25T18:12:31Z

cranelift/codegen/src/isa/s390x/inst.isle

+(decl emit_safepoint (MInst) Unit)
+(extern constructor emit_safepoint emit_safepoint)


And this should be in the prelude as well.

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.

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.

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.

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?)

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.

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.

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.

uweigand · 2022-01-31T15:38:53Z

Can this be merged now or are you waiting on any action from my side? I have other patches I'd like to submit and this is causing merge conflicts ...

As to the question of moving emit_safepoint to the common prelude, my preference would still be for other architecture maintainers to implement this for their platform as fits them best, and once it is done for all, we can still merge any definitions that end up the same back into the common prelude ...

fitzgen · 2022-01-31T18:05:02Z

Can this be merged now or are you waiting on any action from my side?

I think just the nitpicks that we didn't already discuss and agree could wait for follow ups:

moving emit_safepoint into the prelude, and
moving its external rust implementation into wasmtime/cranelift/codegen/src/machinst/isle.rs.

uweigand · 2022-01-31T18:42:19Z

Can this be merged now or are you waiting on any action from my side?

I think just the nitpicks that we didn't already discuss and agree could wait for follow ups:
* moving `emit_safepoint` into the prelude, and

* moving its external rust implementation into `wasmtime/cranelift/codegen/src/machinst/isle.rs`.

But that was just what we discussed: As I understand, x64 will need a different emit_safepoint implementation due the move mitosis stuff, so it's not clear this can be shared.

fitzgen · 2022-01-31T19:26:45Z

Ah right. Then just the decl and extern bits need to move to prelude.isle and the implementations can be pre-backend. We can stub out the other backends with unimplemented!() for now.

uweigand · 2022-01-31T20:48:13Z

Ah right. Then just the decl and extern bits need to move to prelude.isle and the implementations can be pre-backend. We can stub out the other backends with unimplemented!() for now.

I'm happy to do that, but I'd really prefer this to be a separate PR. That will still touch all backends and their generated files, and I'd prefer not to entangle this with this back-end change, which is already quite large as-is ...

fitzgen

Okay, we can do that in follow ups then.

uweigand · 2022-01-31T21:59:50Z

Okay, we can do that in follow ups then.

Thanks! The follow up is now here: #3745

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. labels Jan 25, 2022

fitzgen reviewed Jan 25, 2022

View reviewed changes

fitzgen approved these changes Jan 31, 2022

View reviewed changes

fitzgen merged commit 3c2f695 into bytecodealliance:main Jan 31, 2022

uweigand mentioned this pull request Jan 31, 2022

Move emit and emit_safepoint to prelude.isle #3745

Merged

uweigand deleted the s390x-isle-branchtrap branch January 31, 2022 21:59

		(decl emit_safepoint (MInst) Unit)
		(extern constructor emit_safepoint emit_safepoint)

Conversation

uweigand commented Jan 25, 2022

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uweigand commented Jan 31, 2022

Uh oh!

fitzgen commented Jan 31, 2022

Uh oh!

uweigand commented Jan 31, 2022

Uh oh!

fitzgen commented Jan 31, 2022

Uh oh!

uweigand commented Jan 31, 2022

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

uweigand commented Jan 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants