From 04b30acad984ffacafcbfa9ad3be2eba1409092a Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Wed, 5 Oct 2022 10:35:59 -0700 Subject: [PATCH] Misc cleanups (#5014) * Replace resize+copy_from_slice with extend_from_slice Vec::resize initializes the new space, which is wasted effort if we're just going to call `copy_from_slice` on it immediately afterward. Using `extend_from_slice` is simpler, and very slightly faster. If the new size were bigger than the buffer we're copying from, then it would make sense to initialize the excess. But it isn't: it's always exactly the same size. * Move helpers from Context to CompiledCode These methods only use information from Context::compiled_code, so they should live on CompiledCode instead. * Remove an unnecessary #[cfg_attr] There are other uses of `#[allow(clippy::too_many_arguments)]` in this file, so apparently it doesn't need to be guarded by the "cargo-clippy" feature. * Fix a few comments Two of these were wrong/misleading: - `FunctionBuilder::new` does not clear the provided func_ctx. It does debug-assert that the context is already clear, but I don't think that's worth a comment. - `switch_to_block` does not "create values for the arguments." That's done by the combination of `append_block_params_for_function_params` and `declare_wasm_parameters`. * wasmtime-cranelift: Misc cleanups The main change is to use the `CompiledCode` reference we already had instead of getting it out of `Context` repeatedly. This removes a bunch of `unwrap()` calls. * wasmtime-cranelift: Factor out uncached compile --- cranelift/codegen/src/context.rs | 24 ++----- .../src/isa/aarch64/inst/unwind/systemv.rs | 8 +-- cranelift/codegen/src/isa/mod.rs | 14 +--- .../src/isa/riscv64/inst/unwind/systemv.rs | 8 +-- .../src/isa/s390x/inst/unwind/systemv.rs | 8 +-- .../src/isa/x64/inst/unwind/systemv.rs | 8 +-- cranelift/codegen/src/machinst/mod.rs | 30 ++++++++ cranelift/filetests/src/test_unwind.rs | 4 +- cranelift/wasm/src/environ/spec.rs | 2 +- cranelift/wasm/src/func_translator.rs | 5 +- crates/cranelift/src/compiler.rs | 68 ++++++++----------- 11 files changed, 86 insertions(+), 93 deletions(-) diff --git a/cranelift/codegen/src/context.rs b/cranelift/codegen/src/context.rs index 1003e57337..b136d220df 100644 --- a/cranelift/codegen/src/context.rs +++ b/cranelift/codegen/src/context.rs @@ -119,10 +119,7 @@ impl Context { mem: &mut Vec, ) -> CompileResult<&CompiledCode> { let compiled_code = self.compile(isa)?; - let code_info = compiled_code.code_info(); - let old_len = mem.len(); - mem.resize(old_len + code_info.total_size as usize, 0); - mem[old_len..].copy_from_slice(compiled_code.code_buffer()); + mem.extend_from_slice(compiled_code.code_buffer()); Ok(compiled_code) } @@ -194,32 +191,21 @@ impl Context { /// If available, return information about the code layout in the /// final machine code: the offsets (in bytes) of each basic-block /// start, and all basic-block edges. + #[deprecated = "use CompiledCode::get_code_bb_layout"] pub fn get_code_bb_layout(&self) -> Option<(Vec, Vec<(usize, usize)>)> { - if let Some(result) = self.compiled_code.as_ref() { - Some(( - result.bb_starts.iter().map(|&off| off as usize).collect(), - result - .bb_edges - .iter() - .map(|&(from, to)| (from as usize, to as usize)) - .collect(), - )) - } else { - None - } + self.compiled_code().map(CompiledCode::get_code_bb_layout) } /// Creates unwind information for the function. /// /// Returns `None` if the function has no unwind information. #[cfg(feature = "unwind")] + #[deprecated = "use CompiledCode::create_unwind_info"] pub fn create_unwind_info( &self, isa: &dyn TargetIsa, ) -> CodegenResult> { - let unwind_info_kind = isa.unwind_info_kind(); - let result = self.compiled_code.as_ref().unwrap(); - isa.emit_unwind_info(result, unwind_info_kind) + self.compiled_code().unwrap().create_unwind_info(isa) } /// Run the verifier on the function. diff --git a/cranelift/codegen/src/isa/aarch64/inst/unwind/systemv.rs b/cranelift/codegen/src/isa/aarch64/inst/unwind/systemv.rs index 02c3e9dcf5..6ff9f10d3b 100644 --- a/cranelift/codegen/src/isa/aarch64/inst/unwind/systemv.rs +++ b/cranelift/codegen/src/isa/aarch64/inst/unwind/systemv.rs @@ -91,9 +91,9 @@ mod tests { Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)), )); - context.compile(&*isa).expect("expected compilation"); + let code = context.compile(&*isa).expect("expected compilation"); - let fde = match context + let fde = match code .create_unwind_info(isa.as_ref()) .expect("can create unwind info") { @@ -130,9 +130,9 @@ mod tests { let mut context = Context::for_function(create_multi_return_function(CallConv::SystemV)); - context.compile(&*isa).expect("expected compilation"); + let code = context.compile(&*isa).expect("expected compilation"); - let fde = match context + let fde = match code .create_unwind_info(isa.as_ref()) .expect("can create unwind info") { diff --git a/cranelift/codegen/src/isa/mod.rs b/cranelift/codegen/src/isa/mod.rs index 259d2891b9..ebe1e87c9f 100644 --- a/cranelift/codegen/src/isa/mod.rs +++ b/cranelift/codegen/src/isa/mod.rs @@ -56,7 +56,7 @@ use crate::CodegenResult; use alloc::{boxed::Box, vec::Vec}; use core::fmt; use core::fmt::{Debug, Formatter}; -use target_lexicon::{triple, Architecture, OperatingSystem, PointerWidth, Triple}; +use target_lexicon::{triple, Architecture, PointerWidth, Triple}; // This module is made public here for benchmarking purposes. No guarantees are // made regarding API stability. @@ -360,18 +360,6 @@ impl<'a> dyn TargetIsa + 'a { pointer_width: self.pointer_width(), } } - - /// Returns the flavor of unwind information emitted for this target. - pub(crate) fn unwind_info_kind(&self) -> UnwindInfoKind { - match self.triple().operating_system { - #[cfg(feature = "unwind")] - OperatingSystem::Windows => UnwindInfoKind::Windows, - #[cfg(feature = "unwind")] - _ => UnwindInfoKind::SystemV, - #[cfg(not(feature = "unwind"))] - _ => UnwindInfoKind::None, - } - } } impl Debug for &dyn TargetIsa { diff --git a/cranelift/codegen/src/isa/riscv64/inst/unwind/systemv.rs b/cranelift/codegen/src/isa/riscv64/inst/unwind/systemv.rs index 98ce93876d..45851c8aa9 100644 --- a/cranelift/codegen/src/isa/riscv64/inst/unwind/systemv.rs +++ b/cranelift/codegen/src/isa/riscv64/inst/unwind/systemv.rs @@ -89,9 +89,9 @@ mod tests { Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)), )); - context.compile(&*isa).expect("expected compilation"); + let code = context.compile(&*isa).expect("expected compilation"); - let fde = match context + let fde = match code .create_unwind_info(isa.as_ref()) .expect("can create unwind info") { @@ -129,9 +129,9 @@ mod tests { let mut context = Context::for_function(create_multi_return_function(CallConv::SystemV)); - context.compile(&*isa).expect("expected compilation"); + let code = context.compile(&*isa).expect("expected compilation"); - let fde = match context + let fde = match code .create_unwind_info(isa.as_ref()) .expect("can create unwind info") { diff --git a/cranelift/codegen/src/isa/s390x/inst/unwind/systemv.rs b/cranelift/codegen/src/isa/s390x/inst/unwind/systemv.rs index f787a95d02..61f557fd25 100644 --- a/cranelift/codegen/src/isa/s390x/inst/unwind/systemv.rs +++ b/cranelift/codegen/src/isa/s390x/inst/unwind/systemv.rs @@ -122,9 +122,9 @@ mod tests { Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)), )); - context.compile(&*isa).expect("expected compilation"); + let code = context.compile(&*isa).expect("expected compilation"); - let fde = match context + let fde = match code .create_unwind_info(isa.as_ref()) .expect("can create unwind info") { @@ -164,9 +164,9 @@ mod tests { Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)), )); - context.compile(&*isa).expect("expected compilation"); + let code = context.compile(&*isa).expect("expected compilation"); - let fde = match context + let fde = match code .create_unwind_info(isa.as_ref()) .expect("can create unwind info") { diff --git a/cranelift/codegen/src/isa/x64/inst/unwind/systemv.rs b/cranelift/codegen/src/isa/x64/inst/unwind/systemv.rs index 335c97a107..e49ad27b03 100644 --- a/cranelift/codegen/src/isa/x64/inst/unwind/systemv.rs +++ b/cranelift/codegen/src/isa/x64/inst/unwind/systemv.rs @@ -118,9 +118,9 @@ mod tests { Some(StackSlotData::new(StackSlotKind::ExplicitSlot, 64)), )); - context.compile(&*isa).expect("expected compilation"); + let code = context.compile(&*isa).expect("expected compilation"); - let fde = match context + let fde = match code .create_unwind_info(isa.as_ref()) .expect("can create unwind info") { @@ -157,9 +157,9 @@ mod tests { let mut context = Context::for_function(create_multi_return_function(CallConv::SystemV)); - context.compile(&*isa).expect("expected compilation"); + let code = context.compile(&*isa).expect("expected compilation"); - let fde = match context + let fde = match code .create_unwind_info(isa.as_ref()) .expect("can create unwind info") { diff --git a/cranelift/codegen/src/machinst/mod.rs b/cranelift/codegen/src/machinst/mod.rs index 36e96c708d..044eddddc5 100644 --- a/cranelift/codegen/src/machinst/mod.rs +++ b/cranelift/codegen/src/machinst/mod.rs @@ -358,6 +358,36 @@ pub type CompiledCodeStencil = CompiledCodeBase; /// consumption. pub type CompiledCode = CompiledCodeBase; +impl CompiledCode { + /// If available, return information about the code layout in the + /// final machine code: the offsets (in bytes) of each basic-block + /// start, and all basic-block edges. + pub fn get_code_bb_layout(&self) -> (Vec, Vec<(usize, usize)>) { + ( + self.bb_starts.iter().map(|&off| off as usize).collect(), + self.bb_edges + .iter() + .map(|&(from, to)| (from as usize, to as usize)) + .collect(), + ) + } + + /// Creates unwind information for the function. + /// + /// Returns `None` if the function has no unwind information. + #[cfg(feature = "unwind")] + pub fn create_unwind_info( + &self, + isa: &dyn crate::isa::TargetIsa, + ) -> CodegenResult> { + let unwind_info_kind = match isa.triple().operating_system { + target_lexicon::OperatingSystem::Windows => UnwindInfoKind::Windows, + _ => UnwindInfoKind::SystemV, + }; + isa.emit_unwind_info(self, unwind_info_kind) + } +} + /// An object that can be used to create the text section of an executable. /// /// This primarily handles resolving relative relocations at diff --git a/cranelift/filetests/src/test_unwind.rs b/cranelift/filetests/src/test_unwind.rs index ba53cfd8ee..5174b91da0 100644 --- a/cranelift/filetests/src/test_unwind.rs +++ b/cranelift/filetests/src/test_unwind.rs @@ -39,10 +39,10 @@ impl SubTest for TestUnwind { let isa = context.isa.expect("unwind needs an ISA"); let mut comp_ctx = cranelift_codegen::Context::for_function(func.into_owned()); - comp_ctx.compile(isa).expect("failed to compile function"); + let code = comp_ctx.compile(isa).expect("failed to compile function"); let mut text = String::new(); - match comp_ctx.create_unwind_info(isa).expect("unwind info") { + match code.create_unwind_info(isa).expect("unwind info") { Some(UnwindInfo::WindowsX64(info)) => { let mut mem = vec![0; info.emit_size()]; info.emit(&mut mem); diff --git a/cranelift/wasm/src/environ/spec.rs b/cranelift/wasm/src/environ/spec.rs index bfdaa2b426..2584630f77 100644 --- a/cranelift/wasm/src/environ/spec.rs +++ b/cranelift/wasm/src/environ/spec.rs @@ -165,7 +165,7 @@ pub trait FuncEnvironment: TargetEnvironment { /// The signature `sig_ref` was previously created by `make_indirect_sig()`. /// /// Return the call instruction whose results are the WebAssembly return values. - #[cfg_attr(feature = "cargo-clippy", allow(clippy::too_many_arguments))] + #[allow(clippy::too_many_arguments)] fn translate_call_indirect( &mut self, builder: &mut FunctionBuilder, diff --git a/cranelift/wasm/src/func_translator.rs b/cranelift/wasm/src/func_translator.rs index 20fc76d7ac..43c3b2d760 100644 --- a/cranelift/wasm/src/func_translator.rs +++ b/cranelift/wasm/src/func_translator.rs @@ -94,12 +94,11 @@ impl FuncTranslator { debug_assert_eq!(func.dfg.num_blocks(), 0, "Function must be empty"); debug_assert_eq!(func.dfg.num_insts(), 0, "Function must be empty"); - // This clears the `FunctionBuilderContext`. let mut builder = FunctionBuilder::new(func, &mut self.func_ctx); builder.set_srcloc(cur_srcloc(&reader)); let entry_block = builder.create_block(); builder.append_block_params_for_function_params(entry_block); - builder.switch_to_block(entry_block); // This also creates values for the arguments. + builder.switch_to_block(entry_block); builder.seal_block(entry_block); // Declare all predecessors known. // Make sure the entry block is inserted in the layout before we make any callbacks to @@ -183,7 +182,7 @@ fn parse_local_decls( /// Declare `count` local variables of the same type, starting from `next_local`. /// -/// Fail of too many locals are declared in the function, or if the type is not valid for a local. +/// Fail if too many locals are declared in the function, or if the type is not valid for a local. fn declare_locals( builder: &mut FunctionBuilder, count: u32, diff --git a/crates/cranelift/src/compiler.rs b/crates/cranelift/src/compiler.rs index 40be48f546..6c6c10b154 100644 --- a/crates/cranelift/src/compiler.rs +++ b/crates/cranelift/src/compiler.rs @@ -142,8 +142,7 @@ impl Compiler { } fn get_function_address_map( - &self, - context: &Context, + compiled_code: &CompiledCode, body: &FunctionBody<'_>, body_len: u32, tunables: &Tunables, @@ -162,9 +161,7 @@ impl Compiler { let instructions = if tunables.generate_address_map { collect_address_maps( body_len, - context - .compiled_code() - .unwrap() + compiled_code .buffer .get_srclocs_sorted() .into_iter() @@ -270,46 +267,46 @@ impl wasmtime_environ::Compiler for Compiler { context.func.stack_limit = Some(stack_limit); let FunctionBodyData { validator, body } = input; let mut validator = validator.into_validator(validator_allocations); - func_translator.translate_body( - &mut validator, - body.clone(), - &mut context.func, - &mut func_env, - )?; - - let (code, code_buf) = compile_maybe_cached(&mut context, isa, cache_ctx.as_mut())?; - let alignment = code.alignment; + func_translator.translate_body(&mut validator, body, &mut context.func, &mut func_env)?; + let (_, code_buf) = compile_maybe_cached(&mut context, isa, cache_ctx.as_mut())?; + // compile_maybe_cached returns the compiled_code but that borrow has the same lifetime as + // the mutable borrow of `context`, so the borrow checker prohibits other borrows from + // `context` while it's alive. Borrow it again to make the borrow checker happy. let compiled_code = context.compiled_code().unwrap(); + let alignment = compiled_code.alignment; + let func_relocs = compiled_code .buffer .relocs() .into_iter() .map(|item| mach_reloc_to_reloc(&context.func, item)) - .collect::>(); + .collect(); let traps = compiled_code .buffer .traps() .into_iter() .map(mach_trap_to_trap) - .collect::>(); + .collect(); let stack_maps = mach_stack_maps_to_stack_maps(compiled_code.buffer.stack_maps()); let unwind_info = if isa.flags().unwind_info() { - context + compiled_code .create_unwind_info(isa) .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))? } else { None }; + let length = u32::try_from(code_buf.len()).unwrap(); + let address_transform = - self.get_function_address_map(&context, &body, code_buf.len() as u32, tunables); + Self::get_function_address_map(compiled_code, &body, length, tunables); let ranges = if tunables.generate_native_debuginfo { - Some(context.compiled_code().unwrap().value_labels_ranges.clone()) + Some(compiled_code.value_labels_ranges.clone()) } else { None }; @@ -318,8 +315,6 @@ impl wasmtime_environ::Compiler for Compiler { log::debug!("{:?} translated in {:?}", func_index, timing.total()); log::trace!("{:?} timing info\n{}", func_index, timing); - let length = u32::try_from(code_buf.len()).unwrap(); - let sized_stack_slots = std::mem::take(&mut context.func.sized_stack_slots); self.save_context(CompilerContext { @@ -496,18 +491,9 @@ mod incremental_cache { isa: &dyn TargetIsa, cache_ctx: Option<&mut IncrementalCacheContext>, ) -> Result<(&'a CompiledCode, Vec), CompileError> { - let mut code_buf = Vec::new(); let cache_ctx = match cache_ctx { Some(ctx) => ctx, - None => { - let compiled_code = - context - .compile_and_emit(isa, &mut code_buf) - .map_err(|error| { - CompileError::Codegen(pretty_error(&error.func, error.inner)) - })?; - return Ok((compiled_code, code_buf)); - } + None => return compile_uncached(context, isa), }; let mut cache_store = CraneliftCacheStore(cache_ctx.cache_store.clone()); @@ -521,10 +507,7 @@ mod incremental_cache { cache_ctx.num_cached += 1; } - code_buf.resize(compiled_code.code_info().total_size as _, 0); - code_buf.copy_from_slice(compiled_code.code_buffer()); - - Ok((compiled_code, code_buf)) + Ok((compiled_code, compiled_code.code_buffer().to_vec())) } } @@ -536,12 +519,19 @@ fn compile_maybe_cached<'a>( context: &'a mut Context, isa: &dyn TargetIsa, _cache_ctx: Option<&mut IncrementalCacheContext>, +) -> Result<(&'a CompiledCode, Vec), CompileError> { + compile_uncached(context, isa) +} + +fn compile_uncached<'a>( + context: &'a mut Context, + isa: &dyn TargetIsa, ) -> Result<(&'a CompiledCode, Vec), CompileError> { let mut code_buf = Vec::new(); let compiled_code = context .compile_and_emit(isa, &mut code_buf) .map_err(|error| CompileError::Codegen(pretty_error(&error.func, error.inner)))?; - return Ok((compiled_code, code_buf)); + Ok((compiled_code, code_buf)) } fn to_flag_value(v: &settings::Value) -> FlagValue { @@ -835,10 +825,10 @@ impl Compiler { .traps() .into_iter() .map(mach_trap_to_trap) - .collect::>(); + .collect(); let unwind_info = if isa.flags().unwind_info() { - context + compiled_code .create_unwind_info(isa) .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))? } else { @@ -848,7 +838,7 @@ impl Compiler { Ok(CompiledFunction { body: code_buf, unwind_info, - relocations: Vec::new(), + relocations: Default::default(), sized_stack_slots: Default::default(), value_labels_ranges: Default::default(), info: Default::default(),