Stream: git-wasmtime

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


view this post on Zulip 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.

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:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 06 2023 at 20:37):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 06 2023 at 22:47):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 06 2023 at 22:47):

bjorn3 created PR review comment:

These don't look like they are used.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 06 2023 at 22:53):

bjorn3 submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 06 2023 at 22:54):

bjorn3 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 14:40):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 14:54):

KevinRizzoTO submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 15:52):

saulecabrera requested alexcrichton for a review on PR #5944.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 16:29):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 16:29):

alexcrichton submitted PR review.

view this post on Zulip 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)

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 16:43):

KevinRizzoTO submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 16:46):

KevinRizzoTO submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 16:51):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:13):

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

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:14):

KevinRizzoTO submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:19):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera submitted PR review.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

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

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

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

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

saulecabrera created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:23):

KevinRizzoTO edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:29):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:31):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 17:31):

KevinRizzoTO edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 18:10):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 19:50):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 19:51):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 19:57):

alexcrichton submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 19:57):

alexcrichton submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 19:58):

alexcrichton submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 20:04):

KevinRizzoTO submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 20:12):

KevinRizzoTO submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 20:13):

KevinRizzoTO submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 20:22):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 20:22):

KevinRizzoTO submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 21:20):

KevinRizzoTO edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 22:15):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 22:30):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 22:32):

KevinRizzoTO submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 22:50):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 22:50):

alexcrichton created PR review comment:

Yeah this is the expected fix

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2023 at 22:51):

alexcrichton has enabled auto merge for PR #5944.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2023 at 14:17):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2023 at 15:51):

alexcrichton merged PR #5944.


Last updated: Dec 23 2024 at 12:05 UTC