winch: Refactoring wasmtime compiler integration pieces to share more between Cranelift and Winch#5944
Conversation
KevinRizzoTO
force-pushed
the
compiler-refactor
branch
from
5681449 to
c5236d6
Compare
github-actions
Bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "wasmtime:api", "wasmtime:config", "winch"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
DetailsTo modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
It's been awhile since I drafted this but IIRC I got to a point where I started to question the existence of wasmtime-cranelift-shared since so much ended up being shared. Namely I originally thought that the CompiledFunction structure would be wasmtime-cranelift-specific but in the end I think I ended up concluding that winch would want to use CompiledFunction as-is. Either that or I think CompiledFunction could be almost entirely wholesale replaced with MachBuffer's result of compilation. It's been a bit though so I forget...
In any case though this PR as-is seems fine. I'm a little worried about the extent of boilerplate required for winch which is backed by Cranelift but it's not really a technical issue and it's always something that can be burned down later with more refactoring. I think it'd be ok to land this and continue to refactor in-tree as necessary, especially since I don't think there's a good picture on where all the refactoring will end up anyway.
Match cranelift-codegen's build script where if no architecture is explicitly enabled then the host architecture is implicitly enabled.
This commit refactors the `Builder` type to have a type parameter representing the finished ISA with Cranelift and Winch having their own typedefs for `Builder` to represent their own builders. The intention is to use this shared functionality to produce more shared code between the two codegen backends.
This fixes an oversight from the previous commits to use `cranelift-native` to infer flags for the native host when using default settings with Wasmtime.
The `cranelift-codegen` crate doesn't need this and winch wants the same implementation, so shuffle it around so everyone has access to it.
These are easy enough to plumb through with some shared code for Wasmtime.
Just forwarding an isa-specific setting accessor.
I decided to move a couple things around from Alex's initial changes. Instead of having the shared builder do everything, I went back to having each compiler have a distinct builder implementation. I refactored most of the flag setting logic into a single shared location, so we can still reduce the amount of code duplication. With them being separate, we don't need to maintain things like `LinkOpts` which Winch doesn't currently use. We also have an avenue to error when certain flags are sent to Winch if we don't support them. I'm hoping this will make things more maintainable as we build out Winch. I'm still unsure about keeping everything shared in a single crate (`cranelift_shared`). It's starting to feel like this crate is doing too much, which makes it difficult to name. There does seem to be a need for two distinct abstraction: creating the final executable and the handling of shared/ISA flags when building the compiler. I could make them into two separate crates, but there doesn't seem to be enough there yet to justify it.
KevinRizzoTO
force-pushed
the
compiler-refactor
branch
2 times, most recently
from
60b6947 to
8c8e297
Compare
saulecabrera
left a comment
saulecabrera
left a comment
There was a problem hiding this comment.
The winch pieces look good to me. I left a couple of minor suggestions.
Initially (as discussed this with @KevinRizzoTO offline) I was a bit worried about having a crate without a defined scope, as cranelift-shared seems a bit broad to me, but given that we don't have a full picture of how the refactoring is going to play out I agree that we can iterate in-tree and potentially refine this approach as we build more of the integration pieces.
I'm going to defer the final approval to @alexcrichton once the builder conversation gets resolved.
KevinRizzoTO
force-pushed
the
compiler-refactor
branch
4 times, most recently
from
0f02af0 to
576de72
Compare
| pub fn append_func( | ||
| &mut self, | ||
| name: &str, | ||
| func: &'a CompiledFunction, | ||
| body: &[u8], | ||
| alignment: u32, | ||
| unwind_info: Option<&'a UnwindInfo>, | ||
| relocations: &[Relocation], | ||
| resolve_reloc_target: impl Fn(FuncIndex) -> usize, | ||
| ) -> (SymbolId, Range<u64>) { |
There was a problem hiding this 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.
Co-authored-by: Saúl Cabrera <saulecabrera@gmail.com>
KevinRizzoTO
force-pushed
the
compiler-refactor
branch
from
576de72 to
c53501c
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
My guess is that this will continue to be iterated on as winch evolves over time and we see how it shapes up, but I think this is an ok interim point to reach to get the ball rolling on integrating winch into Wasmtime.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
| "wasmtime-fiber", | ||
| "wasmtime-environ", | ||
| "wasmtime-runtime", | ||
| "wasmtime-cranelift-shared", |
There was a problem hiding this 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
There was a problem hiding this comment.
Yeah this is the expected fix
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
@alexcrichton Put in another fix for the last failure, hopefully all good this time 🤞 |
This change introduces handling of traps and relocations in Winch, which was left out in bytecodealliance#6119. In order to so, this change moves the `CompiledFunction` struct to the `wasmtime-cranelift-shared` crate, allowing Cranelift and Winch to operate on a single, shared representation, following some of the ideas discussed in bytecodealliance#5944. Even though Winch doesn't rely on all the fields of `CompiledFunction`, it eventually will. With the addition of relocations and traps it started to be more evident that even if we wanted to have different representations of a compiled function, they would end up being very similar. This change also consolidates where the `traps` and `relocations` of the `CompiledFunction` get created, by introducing a constructor that operates on a `MachBufferFinalized<Final>`, esentially encapsulating this process in a single place for both Winch and Cranelift.
This change introduces handling of traps and relocations in Winch, which was left out in bytecodealliance#6119. In order to so, this change moves the `CompiledFunction` struct to the `wasmtime-cranelift-shared` crate, allowing Cranelift and Winch to operate on a single, shared representation, following some of the ideas discussed in bytecodealliance#5944. Even though Winch doesn't rely on all the fields of `CompiledFunction`, it eventually will. With the addition of relocations and traps it started to be more evident that even if we wanted to have different representations of a compiled function, they would end up being very similar. This change also consolidates where the `traps` and `relocations` of the `CompiledFunction` get created, by introducing a constructor that operates on a `MachBufferFinalized<Final>`, esentially encapsulating this process in a single place for both Winch and Cranelift.
This change introduces handling of traps and relocations in Winch, which was left out in bytecodealliance#6119. In order to so, this change moves the `CompiledFunction` struct to the `wasmtime-cranelift-shared` crate, allowing Cranelift and Winch to operate on a single, shared representation, following some of the ideas discussed in bytecodealliance#5944. Even though Winch doesn't rely on all the fields of `CompiledFunction`, it eventually will. With the addition of relocations and traps it started to be more evident that even if we wanted to have different representations of a compiled function, they would end up being very similar. This change also consolidates where the `traps` and `relocations` of the `CompiledFunction` get created, by introducing a constructor that operates on a `MachBufferFinalized<Final>`, esentially encapsulating this process in a single place for both Winch and Cranelift.
* winch: Handle relocations and traps This change introduces handling of traps and relocations in Winch, which was left out in #6119. In order to so, this change moves the `CompiledFunction` struct to the `wasmtime-cranelift-shared` crate, allowing Cranelift and Winch to operate on a single, shared representation, following some of the ideas discussed in #5944. Even though Winch doesn't rely on all the fields of `CompiledFunction`, it eventually will. With the addition of relocations and traps it started to be more evident that even if we wanted to have different representations of a compiled function, they would end up being very similar. This change also consolidates where the `traps` and `relocations` of the `CompiledFunction` get created, by introducing a constructor that operates on a `MachBufferFinalized<Final>`, esentially encapsulating this process in a single place for both Winch and Cranelift. * Rework the shared `CompiledFunction` This commit reworks the shared `CompiledFunction` struct. The compiled function now contains the essential pieces to derive all the information to create the final object file and to derive the debug information for the function. This commit also decouples the dwarf emission process by introducing a `metadata` field in the `CompiledFunction` struct, which is used as the central structure for dwarf emission.
4 participants
As discussed with @saulecabrera and @alexcrichton, this PR adds in some refactors that will make the initial Winch integration with
wasmtimesmoother. At a high level:wasmtime-cranelift-sharedcrate, which will provide a temporary location to put shared code between Winch and Cranelift while we see what patterns need to be abtractedIsaBuildertraitenable_incremental_compilationmethod on thewasmtime_environ::CompilerBuildertrait return aResultso the implementation can decide if it's supported or notBig thanks to @alexcrichton for helping out here! I ended up using a lot of his first pass (compared to my much messier version 😛).