Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

winch: Drop FuncEnv trait #6443

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ members = [
"fuzz",
"winch",
"winch/codegen",
"winch/environ"
]
exclude = [
'crates/wasi-common/WASI/tools/witx-cli',
Expand Down Expand Up @@ -176,7 +175,6 @@ cranelift = { path = "cranelift/umbrella", version = "0.97.0" }
wasmtime-wasi-http = { path = "crates/wasi-http", version = "=10.0.0" }

winch-codegen = { path = "winch/codegen", version = "=0.8.0" }
winch-environ = { path = "winch/environ", version = "=0.8.0" }
winch-filetests = { path = "winch/filetests" }
winch-test-macros = { path = "winch/test-macros" }

Expand Down
1 change: 0 additions & 1 deletion crates/winch/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ repository = "https://github.com/bytecodealliance/wasmtime"

[dependencies]
winch-codegen = { workspace = true }
winch-environ = { workspace = true }
target-lexicon = { workspace = true }
wasmtime-environ = { workspace = true }
anyhow = { workspace = true }
Expand Down
8 changes: 2 additions & 6 deletions crates/winch/src/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@ use wasmparser::FuncValidatorAllocations;
use wasmtime_cranelift_shared::{CompiledFunction, ModuleTextBuilder};
use wasmtime_environ::{
CompileError, DefinedFuncIndex, FilePos, FuncIndex, FunctionBodyData, FunctionLoc,
ModuleTranslation, ModuleTypes, PrimaryMap, TrapEncodingBuilder, Tunables, VMOffsets,
WasmFunctionInfo,
ModuleTranslation, ModuleTypes, PrimaryMap, TrapEncodingBuilder, Tunables, WasmFunctionInfo,
};
use winch_codegen::{TargetIsa, TrampolineKind};
use winch_environ::FuncEnv;

pub(crate) struct Compiler {
isa: Box<dyn TargetIsa>,
Expand Down Expand Up @@ -68,11 +66,9 @@ impl wasmtime_environ::Compiler for Compiler {
.unwrap(),
);
let mut validator = validator.into_validator(self.take_allocations());
let vmoffsets = VMOffsets::new(self.isa.pointer_bytes(), &translation.module);
let env = FuncEnv::new(&translation.module, translation.get_types());
let buffer = self
.isa
.compile_function(&sig, &body, &vmoffsets, &env, &mut validator)
.compile_function(&sig, &body, &translation, &mut validator)
.map_err(|e| CompileError::Codegen(format!("{e:?}")));
self.save_allocations(validator.into_allocations());
let buffer = buffer?;
Expand Down
1 change: 0 additions & 1 deletion scripts/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ const CRATES_TO_PUBLISH: &[&str] = &[
"wasmtime-jit",
"wasmtime-cache",
"winch-codegen",
"winch-environ",
"wasmtime-winch",
"wasmtime",
// wasi-common/wiggle
Expand Down
1 change: 0 additions & 1 deletion winch/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ path = "src/main.rs"

[dependencies]
winch-codegen = { workspace = true }
winch-environ = { workspace = true }
winch-filetests = { workspace = true }
winch-test-macros = { workspace = true }
wasmtime-environ = { workspace = true }
Expand Down
43 changes: 37 additions & 6 deletions winch/codegen/src/codegen/env.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,41 @@
use wasmparser::FuncType;
use wasmtime_environ::{FuncIndex, ModuleTranslation, PtrSize, VMOffsets};

/// Function environment used the by the code generation to
/// resolve module and runtime-specific information.
pub trait FuncEnv {
/// Get the callee information from a given function index.
fn callee_from_index(&self, index: u32) -> Callee;
/// The function environment.
///
/// Contains all information about the module and runtime that is accessible to
/// to a particular function during code generation.
pub struct FuncEnv<'a, P> {
/// Offsets to the fields within the `VMContext` ptr.
pub vmoffsets: VMOffsets<P>,
/// Metadata about the translation process of a WebAssembly module.
pub translation: &'a ModuleTranslation<'a>,
}

impl<'a, P: PtrSize> FuncEnv<'a, P> {
/// Create a new function environment.
pub fn new(ptr: P, translation: &'a ModuleTranslation) -> Self {
let vmoffsets = VMOffsets::new(ptr, &translation.module);
Self {
vmoffsets,
translation,
}
}

/// Resolves a function [`Callee`] from an index.
pub fn callee_from_index(&self, idx: FuncIndex) -> Callee {
let types = &self.translation.get_types();
let ty = types
.function_at(idx.as_u32())
.unwrap_or_else(|| panic!("function type at index: {}", idx.as_u32()));
let import = self.translation.module.is_imported_function(idx);

Callee {
ty: ty.clone(),
import,
index: idx,
}
}
}

/// Metadata about a function callee. Use by the code generation
Expand All @@ -15,5 +46,5 @@ pub struct Callee {
/// A flag to determine if the callee is imported.
pub import: bool,
/// The callee index in the WebAssembly function index space.
pub index: u32,
pub index: FuncIndex,
}
27 changes: 12 additions & 15 deletions winch/codegen/src/codegen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use call::FnCall;
use wasmparser::{
BinaryReader, FuncType, FuncValidator, ValType, ValidatorResources, VisitOperator,
};
use wasmtime_environ::{FuncIndex, VMOffsets};
use wasmtime_environ::{FuncIndex, PtrSize};

mod context;
pub(crate) use context::*;
Expand All @@ -18,50 +18,47 @@ pub use env::*;
pub mod call;

/// The code generation abstraction.
pub(crate) struct CodeGen<'a, A, M>
pub(crate) struct CodeGen<'a, A, M, P>
where
M: MacroAssembler,
A: ABI,
P: PtrSize,
Comment on lines 23 to +25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not all that familiar with the design of these generics, but I would naively expect that a choice of either MacroAssembler or ABI would imply PtrSize. If that's the case would it perhaps be possible to have P be an associated type of either prior trait?

Or would it, for example, make sense to put both ABI and PtrSize inside of the MacroAssembler trait as an associated type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Taking a closer look, I'm leaning towards having the PtrSize as an associated type inside the ABI trait, which already provides similar information (e.g. word_bytes).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon a second look, having both of these as associated types in the MacroAssembler makes more sense I think, and it's going to result in quite a decent cut in boilerplate. I started working on the change, but I'm anticipating that it's going to be a decent amount of changes unrelated to this PR, so I think I prefer landing this and putting the refactor once this lands.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also in my todo to standardize the signatures in the ABI trait (e.g. there's no reason to have them borrow &self), so I'm thinking I'll bundle those improvements in the refactoring too.

{
/// The ABI-specific representation of the function signature, excluding results.
sig: ABISig,

/// The code generation context.
pub context: CodeGenContext<'a>,

/// A reference to the function compilation environment.
pub env: FuncEnv<'a, P>,

/// The MacroAssembler.
pub masm: &'a mut M,

/// A reference to the function compilation environment.
pub env: &'a dyn env::FuncEnv,

/// A reference to the current ABI.
pub abi: &'a A,

/// Offsets used with the VM context pointer.
vmoffsets: &'a VMOffsets<u8>,
}

impl<'a, A, M> CodeGen<'a, A, M>
impl<'a, A, M, P> CodeGen<'a, A, M, P>
where
M: MacroAssembler,
A: ABI,
P: PtrSize,
{
pub fn new(
masm: &'a mut M,
abi: &'a A,
context: CodeGenContext<'a>,
env: &'a dyn FuncEnv,
env: FuncEnv<'a, P>,
sig: ABISig,
vmoffsets: &'a VMOffsets<u8>,
) -> Self {
Self {
sig,
context,
masm,
abi,
env,
vmoffsets,
}
}

Expand Down Expand Up @@ -137,7 +134,7 @@ where

/// Emit a direct function call.
pub fn emit_call(&mut self, index: FuncIndex) {
let callee = self.env.callee_from_index(index.as_u32());
let callee = self.env.callee_from_index(index);
let (sig, callee_addr): (ABISig, Option<<M as MacroAssembler>::Address>) = if callee.import
{
let mut params = vec![ValType::I64, ValType::I64];
Expand All @@ -146,14 +143,14 @@ where

let caller_vmctx = <A as ABI>::vmctx_reg();
let callee_vmctx = self.context.any_gpr(self.masm);
let callee_vmctx_offset = self.vmoffsets.vmctx_vmfunction_import_vmctx(index);
let callee_vmctx_offset = self.env.vmoffsets.vmctx_vmfunction_import_vmctx(index);
let callee_vmctx_addr = self.masm.address_at_reg(caller_vmctx, callee_vmctx_offset);
// FIXME Remove harcoded operand size, this will be needed
// once 32-bit architectures are supported.
self.masm
.load(callee_vmctx_addr, callee_vmctx, OperandSize::S64);

let callee_body_offset = self.vmoffsets.vmctx_vmfunction_import_wasm_call(index);
let callee_body_offset = self.env.vmoffsets.vmctx_vmfunction_import_wasm_call(index);
let callee_addr = self.masm.address_at_reg(caller_vmctx, callee_body_offset);

// Put the callee / caller vmctx at the start of the
Expand Down
12 changes: 6 additions & 6 deletions winch/codegen/src/isa/aarch64/mod.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
use self::regs::{scratch, ALL_GPR};
use crate::{
abi::ABI,
codegen::{CodeGen, CodeGenContext},
codegen::{CodeGen, CodeGenContext, FuncEnv},
frame::{DefinedLocals, Frame},
isa::{Builder, CallingConvention, TargetIsa},
masm::MacroAssembler,
regalloc::RegAlloc,
regset::RegSet,
stack::Stack,
FuncEnv, TrampolineKind,
TrampolineKind,
};
use anyhow::Result;
use cranelift_codegen::settings::{self, Flags};
Expand All @@ -17,7 +17,7 @@ use cranelift_codegen::{MachTextSectionBuilder, TextSectionBuilder};
use masm::MacroAssembler as Aarch64Masm;
use target_lexicon::Triple;
use wasmparser::{FuncType, FuncValidator, FunctionBody, ValidatorResources};
use wasmtime_environ::VMOffsets;
use wasmtime_environ::ModuleTranslation;

mod abi;
mod address;
Expand Down Expand Up @@ -86,8 +86,7 @@ impl TargetIsa for Aarch64 {
&self,
sig: &FuncType,
body: &FunctionBody,
vmoffsets: &VMOffsets<u8>,
env: &dyn FuncEnv,
translation: &ModuleTranslation,
validator: &mut FuncValidator<ValidatorResources>,
) -> Result<MachBufferFinalized<Final>> {
let mut body = body.get_binary_reader();
Expand All @@ -101,7 +100,8 @@ impl TargetIsa for Aarch64 {
// TODO: Add floating point bitmask
let regalloc = RegAlloc::new(RegSet::new(ALL_GPR, 0), scratch());
let codegen_context = CodeGenContext::new(regalloc, stack, &frame);
let mut codegen = CodeGen::new(&mut masm, &abi, codegen_context, env, abi_sig, vmoffsets);
let env = FuncEnv::new(self.pointer_bytes(), translation);
let mut codegen = CodeGen::new(&mut masm, &abi, codegen_context, env, abi_sig);

codegen.emit(&mut body, validator)?;
Ok(masm.finalize())
Expand Down
7 changes: 3 additions & 4 deletions winch/codegen/src/isa/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{FuncEnv, TrampolineKind};
use crate::TrampolineKind;
use anyhow::{anyhow, Result};
use core::fmt::Formatter;
use cranelift_codegen::isa::{CallConv, IsaBuilder};
Expand All @@ -10,7 +10,7 @@ use std::{
};
use target_lexicon::{Architecture, Triple};
use wasmparser::{FuncType, FuncValidator, FunctionBody, ValidatorResources};
use wasmtime_environ::VMOffsets;
use wasmtime_environ::ModuleTranslation;

#[cfg(feature = "x64")]
pub(crate) mod x64;
Expand Down Expand Up @@ -149,8 +149,7 @@ pub trait TargetIsa: Send + Sync {
&self,
sig: &FuncType,
body: &FunctionBody,
vmoffsets: &VMOffsets<u8>,
env: &dyn FuncEnv,
translation: &ModuleTranslation,
validator: &mut FuncValidator<ValidatorResources>,
) -> Result<MachBufferFinalized<Final>>;

Expand Down
11 changes: 5 additions & 6 deletions winch/codegen/src/isa/x64/mod.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
use crate::{
abi::ABI,
codegen::{CodeGen, CodeGenContext},
codegen::{CodeGen, CodeGenContext, FuncEnv},
};

use crate::frame::{DefinedLocals, Frame};
use crate::isa::{x64::masm::MacroAssembler as X64Masm, CallingConvention};
use crate::masm::MacroAssembler;
use crate::regalloc::RegAlloc;
use crate::stack::Stack;
use crate::FuncEnv;
use crate::{
isa::{Builder, TargetIsa},
regset::RegSet,
Expand All @@ -20,7 +19,7 @@ use cranelift_codegen::{isa::x64::settings as x64_settings, Final, MachBufferFin
use cranelift_codegen::{MachTextSectionBuilder, TextSectionBuilder};
use target_lexicon::Triple;
use wasmparser::{FuncType, FuncValidator, FunctionBody, ValidatorResources};
use wasmtime_environ::VMOffsets;
use wasmtime_environ::ModuleTranslation;

use self::regs::ALL_GPR;

Expand Down Expand Up @@ -91,8 +90,7 @@ impl TargetIsa for X64 {
&self,
sig: &FuncType,
body: &FunctionBody,
vmoffsets: &VMOffsets<u8>,
env: &dyn FuncEnv,
translation: &ModuleTranslation,
validator: &mut FuncValidator<ValidatorResources>,
) -> Result<MachBufferFinalized<Final>> {
let mut body = body.get_binary_reader();
Expand All @@ -106,7 +104,8 @@ impl TargetIsa for X64 {
// TODO Add in floating point bitmask
let regalloc = RegAlloc::new(RegSet::new(ALL_GPR, 0), regs::scratch());
let codegen_context = CodeGenContext::new(regalloc, stack, &frame);
let mut codegen = CodeGen::new(&mut masm, &abi, codegen_context, env, abi_sig, vmoffsets);
let env = FuncEnv::new(self.pointer_bytes(), translation);
let mut codegen = CodeGen::new(&mut masm, &abi, codegen_context, env, abi_sig);

codegen.emit(&mut body, validator)?;

Expand Down
2 changes: 1 addition & 1 deletion winch/codegen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
#![cfg_attr(not(feature = "all-arch"), allow(dead_code))]

mod abi;
pub use codegen::FuncEnv;
mod codegen;
pub use codegen::{Callee, FuncEnv};
mod frame;
pub mod isa;
pub use isa::*;
Expand Down
Loading