From 63367d205cc129ebcd1a12c5ebdb04bc37ce4353 Mon Sep 17 00:00:00 2001 From: julian-seward1 Date: Tue, 10 Sep 2019 15:06:10 +0200 Subject: [PATCH] Fix #975 (Structure of cranelift-wasm/src/translation_utils.rs causes many pointless heap allocations) (#1010) This patch restricts the `Err(..)` return from `blocktype_to_type` to be `Err(..)` only in the case where it really is an error to continue. The three use points of `blocktype_to_type` are changed to check for an `Err(..)` rather than silently ignoring it. There are also cosmetic changes to `type_to_type` and `tabletype_to_type`. When compiling wasm_lua_binarytrees, this reduces the number of blocks allocated by CL by 1.9%. Instruction count falls by 0.1%. Details: * `type_to_type` and `tabletype_to_type`: - Added the function name in the failure message - No functional change for non-error cases - Push the `Ok(..)` to expression leaves, where it really applies. This corrects the misleading impression that, in the case of an unsupported type, the function returns `Ok` wrapped around whatever `wasm_unsupported` returns. It doesn't do that, but it certainly reads like that. This assumes that the LLVM backend will do tail merging, so the generated code will be unchanged. * `blocktype_to_type`: - Change return type from `WasmResult` to `WasmResult>` - Manually inline the call to `type_to_type`, to make this function easier to read. - For the non-error case: map `TypeOrFuncType::Type(Type::EmptyBlockType)` to `Ok(None)` rather than `Err(..)`, since that's what all the call sites expect - For the error cases, add the function name in the failure messages * cranelift-wasm/src/code_translator.rs - For the three uses of `blocktype_to_type`, use `?` to detect failures and drop out immediately, meaning that the code will no longer silently ignore errors. --- cranelift/wasm/src/code_translator.rs | 6 +-- cranelift/wasm/src/translation_utils.rs | 50 ++++++++++++++----------- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index 664d839023..1801c6539b 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -133,7 +133,7 @@ pub fn translate_operator( ***********************************************************************************/ Operator::Block { ty } => { let next = builder.create_ebb(); - if let Ok(ty_cre) = blocktype_to_type(*ty) { + if let Some(ty_cre) = blocktype_to_type(*ty)? { builder.append_ebb_param(next, ty_cre); } state.push_block(next, num_return_values(*ty)?); @@ -141,7 +141,7 @@ pub fn translate_operator( Operator::Loop { ty } => { let loop_body = builder.create_ebb(); let next = builder.create_ebb(); - if let Ok(ty_cre) = blocktype_to_type(*ty) { + if let Some(ty_cre) = blocktype_to_type(*ty)? { builder.append_ebb_param(next, ty_cre); } builder.ins().jump(loop_body, &[]); @@ -168,7 +168,7 @@ pub fn translate_operator( // and we add nothing; // - either the If have an Else clause, in that case the destination of this jump // instruction will be changed later when we translate the Else operator. - if let Ok(ty_cre) = blocktype_to_type(*ty) { + if let Some(ty_cre) = blocktype_to_type(*ty)? { builder.append_ebb_param(if_not, ty_cre); } state.push_if(jump_inst, if_not, num_return_values(*ty)?); diff --git a/cranelift/wasm/src/translation_utils.rs b/cranelift/wasm/src/translation_utils.rs index 90967d4102..f033b8660c 100644 --- a/cranelift/wasm/src/translation_utils.rs +++ b/cranelift/wasm/src/translation_utils.rs @@ -115,35 +115,43 @@ pub struct Memory { /// Helper function translating wasmparser types to Cranelift types when possible. pub fn type_to_type(ty: wasmparser::Type) -> WasmResult { - Ok(match ty { - wasmparser::Type::I32 => ir::types::I32, - wasmparser::Type::I64 => ir::types::I64, - wasmparser::Type::F32 => ir::types::F32, - wasmparser::Type::F64 => ir::types::F64, - ty => wasm_unsupported!("unsupported wasm type {:?}", ty), - }) + match ty { + wasmparser::Type::I32 => Ok(ir::types::I32), + wasmparser::Type::I64 => Ok(ir::types::I64), + wasmparser::Type::F32 => Ok(ir::types::F32), + wasmparser::Type::F64 => Ok(ir::types::F64), + ty => wasm_unsupported!("type_to_type: wasm type {:?}", ty), + } } /// Helper function translating wasmparser possible table types to Cranelift types when possible, /// or None for Func tables. pub fn tabletype_to_type(ty: wasmparser::Type) -> WasmResult> { - Ok(match ty { - wasmparser::Type::I32 => Some(ir::types::I32), - wasmparser::Type::I64 => Some(ir::types::I64), - wasmparser::Type::F32 => Some(ir::types::F32), - wasmparser::Type::F64 => Some(ir::types::F64), - wasmparser::Type::AnyFunc => None, - ty => wasm_unsupported!("unsupported table wasm type {:?}", ty), - }) + match ty { + wasmparser::Type::I32 => Ok(Some(ir::types::I32)), + wasmparser::Type::I64 => Ok(Some(ir::types::I64)), + wasmparser::Type::F32 => Ok(Some(ir::types::F32)), + wasmparser::Type::F64 => Ok(Some(ir::types::F64)), + wasmparser::Type::AnyFunc => Ok(None), + ty => wasm_unsupported!("tabletype_to_type: table wasm type {:?}", ty), + } } /// Helper function translating wasmparser block signatures to Cranelift types when possible. -pub fn blocktype_to_type(ty: wasmparser::TypeOrFuncType) -> WasmResult { - match ty { - wasmparser::TypeOrFuncType::Type(ty) => type_to_type(ty), - wasmparser::TypeOrFuncType::FuncType(_) => { - wasm_unsupported!("multi-value block signature {:?}", ty); - } +pub fn blocktype_to_type(ty_or_ft: wasmparser::TypeOrFuncType) -> WasmResult> { + match ty_or_ft { + wasmparser::TypeOrFuncType::Type(ty) => match ty { + wasmparser::Type::I32 => Ok(Some(ir::types::I32)), + wasmparser::Type::I64 => Ok(Some(ir::types::I64)), + wasmparser::Type::F32 => Ok(Some(ir::types::F32)), + wasmparser::Type::F64 => Ok(Some(ir::types::F64)), + wasmparser::Type::EmptyBlockType => Ok(None), + ty => wasm_unsupported!("blocktype_to_type: type {:?}", ty), + }, + wasmparser::TypeOrFuncType::FuncType(_) => wasm_unsupported!( + "blocktype_to_type: multi-value block signature {:?}", + ty_or_ft + ), } }