Stream: git-wasmtime

Topic: wasmtime / PR #5944 winch: Refactoring wasmtime compiler ...

Wasmtime GitHub notifications bot (Mar 06 2023 at 20:32):

KevinRizzoTO opened PR #5944 from compiler-refactor to main:

<!--

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.
-->

As discussed with @saulecabrera and @alexcrichton, this PR adds in some refactors that will make the initial Winch integration with wasmtime smoother. At a high level:

• move shared code into the new wasmtime-cranelift-shared crate, which will provide a temporary location to put shared code between Winch and Cranelift while we see what patterns need to be abtracted
• combine some Isa operations under a shared IsaBuilder trait
• have the enable_incremental_compilation method on the wasmtime_environ::CompilerBuilder trait return a Result so the implementation can decide if it's supported or not

Big thanks to @alexcrichton for helping out here! I ended up using a lot of his first pass (compared to my much messier version :stuck_out_tongue:).

Wasmtime GitHub notifications bot (Mar 06 2023 at 20:37):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 06 2023 at 22:47):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2023 at 22:47):

bjorn3 created PR review comment:

These don't look like they are used.

Wasmtime GitHub notifications bot (Mar 06 2023 at 22:53):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Mar 06 2023 at 22:53):

bjorn3 created PR review comment:

Why is this no longer consuming the builder? I don't see a reason why you would want to resue the builder across building multiple TargetIsa. Nornally you did either build once or build multiple times but with different settings.

Wasmtime GitHub notifications bot (Mar 06 2023 at 22:54):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Mar 07 2023 at 14:40):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 14:54):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 14:54):

KevinRizzoTO created PR review comment:

Looking through this a bit, it seems like we don't necessarily have to make this change.

However, you can see here that previously, even though we were consuming the builder when calling finish, we ended up cloning the builder anyways to get around moving the value behind a shared reference. It might make more sense to not explicitly move the value out when calling finish if it means the builder is more ergonomic to use in other places. Also cc @alexcrichton in case you have thoughts on this.

Wasmtime GitHub notifications bot (Mar 07 2023 at 15:52):

saulecabrera requested alexcrichton for a review on PR #5944.

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:29):

alexcrichton created PR review comment:

Are these still necessary? (I may be missing the changes where they get applied)

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:29):

alexcrichton created PR review comment:

Ah I've forgotten why I originally did it. At some point along the way when I was originally refactoring I needed to do this to resolve some compile error somewhere but I've lost the context so it might be worth backing out this part and seeing what happens?

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:43):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:43):

KevinRizzoTO created PR review comment:

Ah yeah I had to do a bit of git surgery to split out some of the major winch changes to be included in a later PR, so I think this slipped through. It's been removed

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:46):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:46):

KevinRizzoTO created PR review comment:

Yeah I took a stab at moving back! It can be done but it requires cloning the builder in other places (it's &self all the way down). I don't have a preference really but I can move it back if that's easier.

Wasmtime GitHub notifications bot (Mar 07 2023 at 16:51):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:13):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:14):

KevinRizzoTO created PR review comment:

I gave it a try here and it seems to work fine, but it requires adding clone support in a few more places.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:14):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:19):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

/// Type alias of `IsaBuilder` used for building Cranelift's ISAs.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

        // See `cranelift_codegen`'s value of this for more information.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

    /// See `cranelift_codegen::isa::TargetIsa::create_systemv_cie`.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

        // By default, an ISA cannot create a System V CIE.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

    /// See `cranelift_codegen::isa::TargetIsa::function_alignment`.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

        // See `cranelift_codegen::isa::TargetIsa::function_alignment`.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

        // By default, an ISA cannot create a System V CIE.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

    /// See `cranelift_codegen::isa::TargetIsa::text_section_builder`.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

KevinRizzoTO edited PR review comment.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:29):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:31):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 17:31):

KevinRizzoTO edited PR review comment.

Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:50):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:51):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:57):

alexcrichton created PR review comment:

Not something that needs to be handled here in this PR, but in the future this will need to be invoked by winch and I _think_ that most of the arguments here can simply become the output of MachBuffer's compilation step since I think it's largely all packaged in there already (except maybe alignment IIRC). Just something to keep in mind though as more of the winch bits are filled out.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:57):

alexcrichton created PR review comment:

I see the refactoring in cranelift-native, but I think that the usage of cranelift_native here may be lost? I think part of IsaBuilder::new may want to pass the configured flags to the native configuration crate as well?

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 19:58):

alexcrichton created PR review comment:

Oh sorry forgot to mention just now, but I do not personally have any preference on which way this goes. I don't know of any reason myself to preserve the current functionality nor necessarily any reason to move to the borrowed version.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:04):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:04):

KevinRizzoTO created PR review comment:

Oh yes thanks good catch! This needs to be added back in

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:12):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:12):

KevinRizzoTO created PR review comment:

Ah I see, I didn't realize that the initial version of isa_flags is this builder returned from cranelift_native but any time you call target uses the lookup function from the isa module :thinking: I'll have to rework this a bit so the caller can pass an initial builder that might differ from the one lookup returns.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:13):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:13):

KevinRizzoTO created PR review comment:

So my commit messed a few things on the winch side that's making me think the borrowed version might be a bit easier. Needing to clone the builder in a call is a bit tricky, since the winch and Cranelift currently use different return types with the IsaBuilder. I could unify them in theory, but I'm thinking it might be better to use the borrowing strategy here while we are still figuring what needs to be common.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:22):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:22):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 20:22):

KevinRizzoTO created PR review comment:

Ah nvm on that, I see your previous change with the infer_native_flags. I've added this back in

Wasmtime GitHub notifications bot (Mar 07 2023 at 21:20):

KevinRizzoTO edited PR review comment.

Wasmtime GitHub notifications bot (Mar 07 2023 at 22:15):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 22:30):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 07 2023 at 22:32):

KevinRizzoTO submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 22:32):

KevinRizzoTO created PR review comment:

@alexcrichton @saulecabrera let me know if this works to add this here to pass merge queue CI, or if we should be adding publish = false to the new Cargo.toml file

Wasmtime GitHub notifications bot (Mar 07 2023 at 22:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2023 at 22:50):

alexcrichton created PR review comment:

Yeah this is the expected fix

Wasmtime GitHub notifications bot (Mar 07 2023 at 22:51):

alexcrichton has enabled auto merge for PR #5944.

Wasmtime GitHub notifications bot (Mar 08 2023 at 14:17):

KevinRizzoTO updated PR #5944 from compiler-refactor to main.

Wasmtime GitHub notifications bot (Mar 08 2023 at 15:51):

alexcrichton merged PR #5944.

Last updated: Nov 22 2024 at 16:03 UTC