From e73ac60053acab900385a0917be8bbeb956f8f3a Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 20 Oct 2021 23:49:53 +0200 Subject: [PATCH 01/44] improve Tag literal generation --- compiler/gen_llvm/src/llvm/build.rs | 115 +++++++++++++++------------- 1 file changed, 61 insertions(+), 54 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index bacda91a69..15bba5307b 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -618,8 +618,8 @@ pub fn construct_optimization_passes<'a>( mpm.add_always_inliner_pass(); // tail-call elimination is always on - fpm.add_instruction_combining_pass(); - fpm.add_tail_call_elimination_pass(); + // fpm.add_instruction_combining_pass(); + // fpm.add_tail_call_elimination_pass(); let pmb = PassManagerBuilder::create(); match opt_level { @@ -629,7 +629,7 @@ pub fn construct_optimization_passes<'a>( OptLevel::Optimize => { pmb.set_optimization_level(OptimizationLevel::Aggressive); // this threshold seems to do what we want - pmb.set_inliner_with_threshold(275); + pmb.set_inliner_with_threshold(0); // TODO figure out which of these actually help @@ -1241,17 +1241,26 @@ pub fn build_exp_expr<'a, 'ctx, 'env>( let struct_type = basic_type_from_layout(env, &struct_layout); - let struct_value = access_index_struct_value( - builder, - argument.into_struct_value(), - struct_type.into_struct_type(), + let argument_pointer = builder.build_alloca(argument.get_type(), "cast_alloca"); + builder.build_store(argument_pointer, argument); + + let struct_ptr = builder.build_pointer_cast( + argument_pointer, + struct_type.ptr_type(AddressSpace::Generic), + "foobar", ); + // let struct_value = access_index_struct_value( + // builder, + // argument.into_struct_value(), + // struct_type.into_struct_type(), + // ); + let result = builder - .build_extract_value(struct_value, *index as u32, "") + .build_struct_gep(struct_ptr, *index as u32, "") .expect("desired field did not decode"); - result + builder.build_load(result, "foobarbaz") } UnionLayout::Recursive(tag_layouts) => { debug_assert!(argument.is_pointer_value()); @@ -1453,7 +1462,13 @@ pub fn build_tag<'a, 'ctx, 'env>( UnionLayout::NonRecursive(tags) => { debug_assert!(union_size > 1); - let ctx = env.context; + let internal_type = block_of_memory_slices(env.context, tags, env.ptr_bytes); + + let tag_id_type = basic_type_from_layout(env, &tag_id_layout).into_int_type(); + let wrapper_type = env + .context + .struct_type(&[internal_type, tag_id_type.into()], false); + let result_alloca = env.builder.build_alloca(wrapper_type, "tag_opaque"); // Determine types let num_fields = arguments.len() + 1; @@ -1484,54 +1499,46 @@ pub fn build_tag<'a, 'ctx, 'env>( } } } - - // Create the struct_type - let struct_type = ctx.struct_type(field_types.into_bump_slice(), false); - - // Insert field exprs into struct_val - let struct_val = - struct_from_fields(env, struct_type, field_vals.into_iter().enumerate()); - - // How we create tag values - // - // The memory layout of tags can be different. e.g. in - // - // [ Ok Int, Err Str ] - // - // the `Ok` tag stores a 64-bit integer, the `Err` tag stores a struct. - // All tags of a union must have the same length, for easy addressing (e.g. array lookups). - // So we need to ask for the maximum of all tag's sizes, even if most tags won't use - // all that memory, and certainly won't use it in the same way (the tags have fields of - // different types/sizes) - // - // In llvm, we must be explicit about the type of value we're creating: we can't just - // make a unspecified block of memory. So what we do is create a byte array of the - // desired size. Then when we know which tag we have (which is here, in this function), - // we need to cast that down to the array of bytes that llvm expects - // - // There is the bitcast instruction, but it doesn't work for arrays. So we need to jump - // through some hoops using store and load to get this to work: the array is put into a - // one-element struct, which can be cast to the desired type. - // - // This tricks comes from - // https://github.com/raviqqe/ssf/blob/bc32aae68940d5bddf5984128e85af75ca4f4686/ssf-llvm/src/expression_compiler.rs#L116 - - let internal_type = block_of_memory_slices(env.context, tags, env.ptr_bytes); - - let data = cast_tag_to_block_of_memory(env, struct_val, internal_type); - let tag_id_type = basic_type_from_layout(env, &tag_id_layout).into_int_type(); - let wrapper_type = env - .context - .struct_type(&[data.get_type(), tag_id_type.into()], false); + // store the tag id + let tag_id_ptr = env + .builder + .build_struct_gep(result_alloca, TAG_ID_INDEX, "get_opaque_data") + .unwrap(); let tag_id_intval = tag_id_type.const_int(tag_id as u64, false); + env.builder.build_store(tag_id_ptr, tag_id_intval); - let field_vals = [ - (TAG_DATA_INDEX as usize, data), - (TAG_ID_INDEX as usize, tag_id_intval.into()), - ]; + // Create the struct_type + let struct_type = env + .context + .struct_type(field_types.into_bump_slice(), false); - struct_from_fields(env, wrapper_type, field_vals.iter().copied()).into() + let struct_opaque_ptr = env + .builder + .build_struct_gep(result_alloca, TAG_DATA_INDEX, "get_opaque_data") + .unwrap(); + let struct_ptr = env.builder.build_pointer_cast( + struct_opaque_ptr, + struct_type.ptr_type(AddressSpace::Generic), + "to_specific", + ); + + // Insert field exprs into struct_val + //let struct_val = + //struct_from_fields(env, struct_type, field_vals.into_iter().enumerate()); + + // Insert field exprs into struct_val + for (index, field_val) in field_vals.iter().copied().enumerate() { + let index: u32 = index as u32; + + let ptr = env + .builder + .build_struct_gep(struct_ptr, index, "get_tag_field_ptr") + .unwrap(); + env.builder.build_store(ptr, field_val); + } + + env.builder.build_load(result_alloca, "load_result") } UnionLayout::Recursive(tags) => { debug_assert!(union_size > 1); From 7d1bd0ffe7c562f2191f30e1697052f0a5af1a1b Mon Sep 17 00:00:00 2001 From: Folkert Date: Thu, 21 Oct 2021 22:08:32 +0200 Subject: [PATCH 02/44] make refcount take tag union by reference --- compiler/gen_llvm/src/llvm/build.rs | 2 +- compiler/gen_llvm/src/llvm/refcounting.rs | 61 ++++++++++++++++++----- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 15bba5307b..80c588bfb0 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -971,7 +971,7 @@ pub fn build_exp_call<'a, 'ctx, 'env>( } } -const TAG_ID_INDEX: u32 = 1; +pub const TAG_ID_INDEX: u32 = 1; pub const TAG_DATA_INDEX: u32 = 0; pub fn struct_from_fields<'a, 'ctx, 'env, I>( diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index 8a89c924d7..0e8f275554 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -2,7 +2,7 @@ use crate::debug_info_init; use crate::llvm::bitcode::call_void_bitcode_fn; use crate::llvm::build::{ add_func, cast_basic_basic, cast_block_of_memory_to_tag, get_tag_id, get_tag_id_non_recursive, - tag_pointer_clear_tag_id, Env, FAST_CALL_CONV, TAG_DATA_INDEX, + tag_pointer_clear_tag_id, Env, FAST_CALL_CONV, TAG_DATA_INDEX, TAG_ID_INDEX, }; use crate::llvm::build_list::{incrementing_elem_loop, list_len, load_list}; use crate::llvm::convert::{basic_type_from_layout, ptr_int}; @@ -542,6 +542,24 @@ fn modify_refcount_layout_help<'a, 'ctx, 'env>( } }, _ => { + if let Layout::Union(UnionLayout::NonRecursive(_)) = layout { + let alloca = env.builder.build_alloca(value.get_type(), "tag_union"); + env.builder.build_store(alloca, value); + call_help(env, function, call_mode, alloca.into()); + return; + } + + if let Layout::LambdaSet(lambda_set) = layout { + if let Layout::Union(UnionLayout::NonRecursive(_)) = + lambda_set.runtime_representation() + { + let alloca = env.builder.build_alloca(value.get_type(), "tag_union"); + env.builder.build_store(alloca, value); + call_help(env, function, call_mode, alloca.into()); + return; + } + } + call_help(env, function, call_mode, value); } } @@ -1603,7 +1621,9 @@ fn modify_refcount_union<'a, 'ctx, 'env>( Some(function_value) => function_value, None => { let basic_type = basic_type_from_layout(env, &layout); - let function_value = build_header(env, basic_type, mode, &fn_name); + // we pass tag unions by reference for efficiency + let ptr_type = basic_type.ptr_type(AddressSpace::Generic).into(); + let function_value = build_header(env, ptr_type, mode, &fn_name); modify_refcount_union_help( env, @@ -1647,18 +1667,27 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( // Add args to scope let arg_symbol = Symbol::ARG_1; - let arg_val = fn_val.get_param_iter().next().unwrap(); + let arg_ptr = fn_val.get_param_iter().next().unwrap().into_pointer_value(); - arg_val.set_name(arg_symbol.as_str(&env.interns)); + arg_ptr.set_name(arg_symbol.as_str(&env.interns)); let parent = fn_val; let before_block = env.builder.get_insert_block().expect("to be in a function"); + let arg_val = env.builder.build_load(arg_ptr, "load_tag_union"); let wrapper_struct = arg_val.into_struct_value(); // read the tag_id - let tag_id = get_tag_id_non_recursive(env, wrapper_struct); + let tag_id_ptr = env + .builder + .build_struct_gep(arg_ptr, TAG_ID_INDEX, "tag_id_ptr") + .unwrap(); + + let tag_id = env + .builder + .build_load(tag_id_ptr, "load_tag_id") + .into_int_value(); let tag_id_u8 = env .builder @@ -1686,12 +1715,18 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( let wrapper_type = basic_type_from_layout(env, &Layout::Struct(field_layouts)); debug_assert!(wrapper_type.is_struct_type()); - let data_bytes = env + let opaque_tag_data_ptr = env .builder - .build_extract_value(wrapper_struct, TAG_DATA_INDEX, "read_tag_id") - .unwrap() - .into_struct_value(); - let wrapper_struct = cast_block_of_memory_to_tag(env.builder, data_bytes, wrapper_type); + .build_struct_gep(arg_ptr, TAG_DATA_INDEX, "tag_id_ptr") + .unwrap(); + + let cast_tag_data_pointer = env.builder.build_pointer_cast( + opaque_tag_data_ptr, + wrapper_type.ptr_type(AddressSpace::Generic), + "cast_to_concrete_tag", + ); + + // let wrapper_struct = cast_block_of_memory_to_tag(env.builder, data_bytes, wrapper_type); for (i, field_layout) in field_layouts.iter().enumerate() { if let Layout::RecursivePointer = field_layout { @@ -1699,16 +1734,18 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( } else if field_layout.contains_refcounted() { let field_ptr = env .builder - .build_extract_value(wrapper_struct, i as u32, "modify_tag_field") + .build_struct_gep(cast_tag_data_pointer, i as u32, "modify_tag_field") .unwrap(); + let field_value = env.builder.build_load(field_ptr, "field_value"); + modify_refcount_layout_help( env, parent, layout_ids, mode.to_call_mode(fn_val), when_recursive, - field_ptr, + field_value, field_layout, ); } From 28b15cdf67949402b4e0019a82526be8b4a2a824 Mon Sep 17 00:00:00 2001 From: Folkert Date: Thu, 21 Oct 2021 22:57:22 +0200 Subject: [PATCH 03/44] prettier --- compiler/gen_llvm/src/llvm/build.rs | 4 ++-- compiler/gen_llvm/src/llvm/refcounting.rs | 5 +---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 80c588bfb0..1ec0d634ed 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -618,8 +618,8 @@ pub fn construct_optimization_passes<'a>( mpm.add_always_inliner_pass(); // tail-call elimination is always on - // fpm.add_instruction_combining_pass(); - // fpm.add_tail_call_elimination_pass(); + fpm.add_instruction_combining_pass(); + fpm.add_tail_call_elimination_pass(); let pmb = PassManagerBuilder::create(); match opt_level { diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index 0e8f275554..f183e029f6 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -1675,9 +1675,6 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( let before_block = env.builder.get_insert_block().expect("to be in a function"); - let arg_val = env.builder.build_load(arg_ptr, "load_tag_union"); - let wrapper_struct = arg_val.into_struct_value(); - // read the tag_id let tag_id_ptr = env .builder @@ -1717,7 +1714,7 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( debug_assert!(wrapper_type.is_struct_type()); let opaque_tag_data_ptr = env .builder - .build_struct_gep(arg_ptr, TAG_DATA_INDEX, "tag_id_ptr") + .build_struct_gep(arg_ptr, TAG_DATA_INDEX, "field_ptr") .unwrap(); let cast_tag_data_pointer = env.builder.build_pointer_cast( From 171c0836e4fffd81226bad6bdc1fd0facb1aa5ec Mon Sep 17 00:00:00 2001 From: Folkert Date: Fri, 22 Oct 2021 11:45:24 +0200 Subject: [PATCH 04/44] return tag unions by pointer --- compiler/gen_llvm/src/llvm/build.rs | 121 +++++++++++++++++++++++----- 1 file changed, 101 insertions(+), 20 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 1ec0d634ed..82dfdd2b36 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -629,7 +629,7 @@ pub fn construct_optimization_passes<'a>( OptLevel::Optimize => { pmb.set_optimization_level(OptimizationLevel::Aggressive); // this threshold seems to do what we want - pmb.set_inliner_with_threshold(0); + pmb.set_inliner_with_threshold(275); // TODO figure out which of these actually help @@ -2377,15 +2377,42 @@ pub fn build_exp_stmt<'a, 'ctx, 'env>( result } Ret(symbol) => { - let value = load_symbol(scope, symbol); + let (value, layout) = load_symbol_and_layout(scope, symbol); - if let Some(block) = env.builder.get_insert_block() { - if block.get_terminator().is_none() { - env.builder.build_return(Some(&value)); + match RocReturn::from_layout(env, layout) { + RocReturn::Return => { + if let Some(block) = env.builder.get_insert_block() { + if block.get_terminator().is_none() { + env.builder.build_return(Some(&value)); + } + } + + value + } + RocReturn::ByPointer => { + // we need to write our value into the final argument of the current function + let parameters = parent.get_params(); + let out_parameter = parameters.last().unwrap(); + debug_assert!(out_parameter.is_pointer_value()); + + env.builder + .build_store(out_parameter.into_pointer_value(), value); + + if let Some(block) = env.builder.get_insert_block() { + match block.get_terminator() { + None => { + env.builder.build_return(None); + } + Some(terminator) => { + terminator.remove_from_basic_block(); + env.builder.build_return(None); + } + } + } + + env.context.i8_type().const_zero().into() } } - - value } Switch { @@ -3106,7 +3133,6 @@ fn expose_function_to_host_help_c_abi_generic<'a, 'ctx, 'env>( c_function_name: &str, ) -> FunctionValue<'ctx> { // NOTE we ingore env.is_gen_test here - let wrapper_return_type = roc_function.get_type().get_return_type().unwrap(); let mut cc_argument_types = Vec::with_capacity_in(arguments.len(), env.arena); for layout in arguments { @@ -3116,12 +3142,19 @@ fn expose_function_to_host_help_c_abi_generic<'a, 'ctx, 'env>( // STEP 1: turn `f : a,b,c -> d` into `f : a,b,c, &d -> {}` // let mut argument_types = roc_function.get_type().get_param_types(); let mut argument_types = cc_argument_types; - let return_type = wrapper_return_type; - let output_type = return_type.ptr_type(AddressSpace::Generic); - argument_types.push(output_type.into()); + let c_function_type = match roc_function.get_type().get_return_type() { + None => { + // this function already returns by-pointer + roc_function.get_type() + } + Some(return_type) => { + let output_type = return_type.ptr_type(AddressSpace::Generic); + argument_types.push(output_type.into()); - let c_function_type = env.context.void_type().fn_type(&argument_types, false); + env.context.void_type().fn_type(&argument_types, false) + } + }; let c_function = add_func( env.module, @@ -3167,10 +3200,10 @@ fn expose_function_to_host_help_c_abi_generic<'a, 'ctx, 'env>( let arguments_for_call = &arguments_for_call.into_bump_slice(); - debug_assert_eq!(args.len(), roc_function.get_params().len()); - let call_result = { if env.is_gen_test { + debug_assert_eq!(args.len(), roc_function.get_params().len()); + let roc_wrapper_function = make_exception_catcher(env, roc_function); debug_assert_eq!( arguments_for_call.len(), @@ -3375,7 +3408,8 @@ fn expose_function_to_host_help_c_abi<'a, 'ctx, 'env>( ) .into() } else { - roc_function.get_type().get_return_type().unwrap() + // roc_function.get_type().get_return_type().unwrap() + basic_type_from_layout(env, &return_layout) }; let mut cc_argument_types = Vec::with_capacity_in(arguments.len(), env.arena); @@ -3432,10 +3466,15 @@ fn expose_function_to_host_help_c_abi<'a, 'ctx, 'env>( CCReturn::Void => { debug_assert_eq!(args.len(), roc_function.get_params().len()); } - CCReturn::ByPointer => { - args = &args[..args.len() - 1]; - debug_assert_eq!(args.len(), roc_function.get_params().len()); - } + CCReturn::ByPointer => match RocReturn::from_layout(env, &return_layout) { + RocReturn::ByPointer => { + debug_assert_eq!(args.len(), roc_function.get_params().len()); + } + RocReturn::Return => { + args = &args[..args.len() - 1]; + debug_assert_eq!(args.len(), roc_function.get_params().len()); + } + }, } let mut arguments_for_call = Vec::with_capacity_in(args.len(), env.arena); @@ -3975,7 +4014,17 @@ fn build_proc_header<'a, 'ctx, 'env>( arg_basic_types.push(arg_type); } - let fn_type = ret_type.fn_type(&arg_basic_types, false); + let fn_type = match RocReturn::from_layout(env, &proc.ret_layout) { + RocReturn::Return => ret_type.fn_type(&arg_basic_types, false), + RocReturn::ByPointer => { + println!( + "{:?} will return void instead of {:?}", + symbol, proc.ret_layout + ); + arg_basic_types.push(ret_type.ptr_type(AddressSpace::Generic).into()); + env.context.void_type().fn_type(&arg_basic_types, false) + } + }; let fn_val = add_func( env.module, @@ -4340,6 +4389,7 @@ fn roc_call_with_args<'a, 'ctx, 'env>( let fn_val = function_value_by_func_spec(env, func_spec, symbol, argument_layouts, result_layout); +<<<<<<< HEAD call_roc_function(env, fn_val, result_layout, arguments) } @@ -5690,6 +5740,37 @@ fn to_cc_type_builtin<'a, 'ctx, 'env>( } } +enum RocReturn { + /// Return as normal + Return, + /// require an extra argument, a pointer + /// where the result is written into returns void + ByPointer, +} + +impl RocReturn { + fn roc_return_by_pointer(layout: Layout) -> bool { + match layout { + Layout::Union(union_layout) => match union_layout { + UnionLayout::NonRecursive(_) => true, + _ => false, + }, + Layout::LambdaSet(lambda_set) => { + RocReturn::roc_return_by_pointer(lambda_set.runtime_representation()) + } + _ => false, + } + } + + fn from_layout<'a, 'ctx, 'env>(env: &Env<'a, 'ctx, 'env>, layout: &Layout<'a>) -> Self { + if false && Self::roc_return_by_pointer(*layout) { + RocReturn::ByPointer + } else { + RocReturn::Return + } + } +} + enum CCReturn { /// Return as normal Return, From 2ff3a97adaa85bf2bcc6b02e87d8c4d8c9d401f2 Mon Sep 17 00:00:00 2001 From: Folkert Date: Fri, 22 Oct 2021 13:24:18 +0200 Subject: [PATCH 05/44] re-implement roc returning by pointer --- compiler/gen_llvm/src/llvm/build.rs | 58 ++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 82dfdd2b36..9140b147a9 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -4389,28 +4389,55 @@ fn roc_call_with_args<'a, 'ctx, 'env>( let fn_val = function_value_by_func_spec(env, func_spec, symbol, argument_layouts, result_layout); -<<<<<<< HEAD call_roc_function(env, fn_val, result_layout, arguments) } fn call_roc_function<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, roc_function: FunctionValue<'ctx>, - _result_layout: &Layout<'a>, + result_layout: &Layout<'a>, arguments: &[BasicValueEnum<'ctx>], ) -> BasicValueEnum<'ctx> { - let call = env.builder.build_call(roc_function, arguments, "call"); + match RocReturn::from_layout(env, result_layout) { + RocReturn::Return => { + debug_assert_eq!( + roc_function.get_type().get_param_types().len(), + arguments.len() + ); + let call = env.builder.build_call(roc_function, arguments, "call"); - // roc functions should have the fast calling convention - debug_assert_eq!(roc_function.get_call_conventions(), FAST_CALL_CONV); - call.set_call_convention(FAST_CALL_CONV); + // roc functions should have the fast calling convention + debug_assert_eq!(roc_function.get_call_conventions(), FAST_CALL_CONV); + call.set_call_convention(FAST_CALL_CONV); - call.try_as_basic_value().left().unwrap_or_else(|| { - panic!( - "LLVM error: Invalid call by name for name {:?}", - roc_function.get_name() - ) - }) + call.try_as_basic_value().left().unwrap_or_else(|| { + panic!( + "LLVM error: Invalid call by name for name {:?}", + roc_function.get_name() + ) + }) + } + RocReturn::ByPointer => { + let mut arguments = Vec::from_iter_in(arguments.iter().copied(), env.arena); + + let result_type = basic_type_from_layout(env, result_layout); + let result_alloca = env.builder.build_alloca(result_type, "result_value"); + + arguments.push(result_alloca.into()); + + debug_assert_eq!( + roc_function.get_type().get_param_types().len(), + arguments.len() + ); + let call = env.builder.build_call(roc_function, &arguments, "call"); + + // roc functions should have the fast calling convention + debug_assert_eq!(roc_function.get_call_conventions(), FAST_CALL_CONV); + call.set_call_convention(FAST_CALL_CONV); + + env.builder.build_load(result_alloca, "load_result") + } + } } /// Translates a target_lexicon::Triple to a LLVM calling convention u32 @@ -5751,10 +5778,7 @@ enum RocReturn { impl RocReturn { fn roc_return_by_pointer(layout: Layout) -> bool { match layout { - Layout::Union(union_layout) => match union_layout { - UnionLayout::NonRecursive(_) => true, - _ => false, - }, + Layout::Union(union_layout) => matches!(union_layout, UnionLayout::NonRecursive(_)), Layout::LambdaSet(lambda_set) => { RocReturn::roc_return_by_pointer(lambda_set.runtime_representation()) } @@ -5762,7 +5786,7 @@ impl RocReturn { } } - fn from_layout<'a, 'ctx, 'env>(env: &Env<'a, 'ctx, 'env>, layout: &Layout<'a>) -> Self { + fn from_layout<'a, 'ctx, 'env>(_env: &Env<'a, 'ctx, 'env>, layout: &Layout<'a>) -> Self { if false && Self::roc_return_by_pointer(*layout) { RocReturn::ByPointer } else { From 1d1bd3d0513f3ef810aa54e81f724ea62242a597 Mon Sep 17 00:00:00 2001 From: Folkert Date: Fri, 22 Oct 2021 14:54:15 +0200 Subject: [PATCH 06/44] working, but generates more code --- compiler/gen_llvm/src/llvm/build.rs | 62 +++++++++++++++++++---------- 1 file changed, 42 insertions(+), 20 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 9140b147a9..44c3565104 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -4398,26 +4398,30 @@ fn call_roc_function<'a, 'ctx, 'env>( result_layout: &Layout<'a>, arguments: &[BasicValueEnum<'ctx>], ) -> BasicValueEnum<'ctx> { + let pass_by_pointer = roc_function.get_type().get_param_types().len() == arguments.len() + 1; + match RocReturn::from_layout(env, result_layout) { - RocReturn::Return => { - debug_assert_eq!( - roc_function.get_type().get_param_types().len(), - arguments.len() - ); - let call = env.builder.build_call(roc_function, arguments, "call"); - - // roc functions should have the fast calling convention - debug_assert_eq!(roc_function.get_call_conventions(), FAST_CALL_CONV); - call.set_call_convention(FAST_CALL_CONV); - - call.try_as_basic_value().left().unwrap_or_else(|| { - panic!( - "LLVM error: Invalid call by name for name {:?}", - roc_function.get_name() - ) - }) - } RocReturn::ByPointer => { + if !pass_by_pointer { + let mut arguments = Vec::from_iter_in(arguments.iter().copied(), env.arena); + + let result_type = basic_type_from_layout(env, result_layout); + let result_alloca = env.builder.build_alloca(result_type, "result_value"); + + //arguments.push(result_alloca.into()); + + debug_assert_eq!( + roc_function.get_type().get_param_types().len(), + arguments.len() + ); + let call = env.builder.build_call(roc_function, &arguments, "call"); + + // roc functions should have the fast calling convention + debug_assert_eq!(roc_function.get_call_conventions(), FAST_CALL_CONV); + call.set_call_convention(FAST_CALL_CONV); + + return env.builder.build_load(result_alloca, "load_result"); + } let mut arguments = Vec::from_iter_in(arguments.iter().copied(), env.arena); let result_type = basic_type_from_layout(env, result_layout); @@ -4437,6 +4441,24 @@ fn call_roc_function<'a, 'ctx, 'env>( env.builder.build_load(result_alloca, "load_result") } + RocReturn::Return => { + debug_assert_eq!( + roc_function.get_type().get_param_types().len(), + arguments.len() + ); + let call = env.builder.build_call(roc_function, arguments, "call"); + + // roc functions should have the fast calling convention + debug_assert_eq!(roc_function.get_call_conventions(), FAST_CALL_CONV); + call.set_call_convention(FAST_CALL_CONV); + + call.try_as_basic_value().left().unwrap_or_else(|| { + panic!( + "LLVM error: Invalid call by name for name {:?}", + roc_function.get_name() + ) + }) + } } } @@ -5778,7 +5800,7 @@ enum RocReturn { impl RocReturn { fn roc_return_by_pointer(layout: Layout) -> bool { match layout { - Layout::Union(union_layout) => matches!(union_layout, UnionLayout::NonRecursive(_)), + Layout::Union(UnionLayout::NonRecursive(_)) => true, Layout::LambdaSet(lambda_set) => { RocReturn::roc_return_by_pointer(lambda_set.runtime_representation()) } @@ -5787,7 +5809,7 @@ impl RocReturn { } fn from_layout<'a, 'ctx, 'env>(_env: &Env<'a, 'ctx, 'env>, layout: &Layout<'a>) -> Self { - if false && Self::roc_return_by_pointer(*layout) { + if Self::roc_return_by_pointer(*layout) { RocReturn::ByPointer } else { RocReturn::Return From e378f0d2f997ff66b5af95f01acc961e223ec650 Mon Sep 17 00:00:00 2001 From: Folkert Date: Fri, 5 Nov 2021 10:35:17 +0100 Subject: [PATCH 07/44] fix tags tests --- compiler/gen_llvm/src/llvm/bitcode.rs | 20 +++---- compiler/gen_llvm/src/llvm/build.rs | 82 +++++++++++++++------------ 2 files changed, 54 insertions(+), 48 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/bitcode.rs b/compiler/gen_llvm/src/llvm/bitcode.rs index 52b604dff3..5116f5a033 100644 --- a/compiler/gen_llvm/src/llvm/bitcode.rs +++ b/compiler/gen_llvm/src/llvm/bitcode.rs @@ -191,6 +191,7 @@ pub fn build_transform_caller<'a, 'ctx, 'env>( function: FunctionValue<'ctx>, closure_data_layout: LambdaSet<'a>, argument_layouts: &[Layout<'a>], + result_layout: Layout<'a>, ) -> FunctionValue<'ctx> { let fn_name: &str = &format!( "{}_zig_function_caller", @@ -204,6 +205,7 @@ pub fn build_transform_caller<'a, 'ctx, 'env>( function, closure_data_layout, argument_layouts, + result_layout, fn_name, ), } @@ -214,6 +216,7 @@ fn build_transform_caller_help<'a, 'ctx, 'env>( roc_function: FunctionValue<'ctx>, closure_data_layout: LambdaSet<'a>, argument_layouts: &[Layout<'a>], + result_layout: Layout<'a>, fn_name: &str, ) -> FunctionValue<'ctx> { debug_assert!(argument_layouts.len() <= 7); @@ -288,17 +291,12 @@ fn build_transform_caller_help<'a, 'ctx, 'env>( } } - let call = { - env.builder - .build_call(roc_function, arguments_cast.as_slice(), "tmp") - }; - - call.set_call_convention(FAST_CALL_CONV); - - let result = call - .try_as_basic_value() - .left() - .unwrap_or_else(|| panic!("LLVM error: Invalid call by pointer.")); + let result = crate::llvm::build::call_roc_function( + env, + roc_function, + &result_layout, + arguments_cast.as_slice(), + ); let result_u8_ptr = function_value .get_nth_param(argument_layouts.len() as u32 + 1) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index ed17333c28..242695fa0e 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -3216,7 +3216,7 @@ fn expose_function_to_host_help_c_abi_generic<'a, 'ctx, 'env>( if env.is_gen_test { debug_assert_eq!(args.len(), roc_function.get_params().len()); - let roc_wrapper_function = make_exception_catcher(env, roc_function); + let roc_wrapper_function = make_exception_catcher(env, roc_function, return_layout); debug_assert_eq!( arguments_for_call.len(), roc_wrapper_function.get_params().len() @@ -3261,7 +3261,7 @@ fn expose_function_to_host_help_c_abi_gen_test<'a, 'ctx, 'env>( let wrapper_return_type = context.struct_type( &[ context.i64_type().into(), - roc_function.get_type().get_return_type().unwrap(), + basic_type_from_layout(env, &return_layout), ], false, ); @@ -3326,18 +3326,14 @@ fn expose_function_to_host_help_c_abi_gen_test<'a, 'ctx, 'env>( let arguments_for_call = &arguments_for_call.into_bump_slice(); let call_result = { - let roc_wrapper_function = make_exception_catcher(env, roc_function); - debug_assert_eq!( - arguments_for_call.len(), - roc_wrapper_function.get_params().len() - ); + let roc_wrapper_function = make_exception_catcher(env, roc_function, return_layout); builder.position_at_end(entry); call_roc_function( env, roc_wrapper_function, - &return_layout, + &Layout::Struct(&[Layout::Builtin(Builtin::Int64), return_layout]), arguments_for_call, ) }; @@ -3574,21 +3570,18 @@ pub fn get_sjlj_buffer<'a, 'ctx, 'env>(env: &Env<'a, 'ctx, 'env>) -> PointerValu .into_pointer_value() } -fn set_jump_and_catch_long_jump<'a, 'ctx, 'env, F, T>( +fn set_jump_and_catch_long_jump<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, parent: FunctionValue<'ctx>, - function: F, - calling_convention: u32, + roc_function: FunctionValue<'ctx>, arguments: &[BasicValueEnum<'ctx>], - return_type: T, -) -> BasicValueEnum<'ctx> -where - T: inkwell::types::BasicType<'ctx>, - F: Into>, -{ + return_layout: Layout<'a>, +) -> BasicValueEnum<'ctx> { let context = env.context; let builder = env.builder; + let return_type = basic_type_from_layout(env, &return_layout); + let call_result_type = context.struct_type( &[context.i64_type().into(), return_type.as_basic_type_enum()], false, @@ -3661,11 +3654,7 @@ where { builder.position_at_end(then_block); - let call = env.builder.build_call(function, arguments, "call_function"); - - call.set_call_convention(calling_convention); - - let call_result = call.try_as_basic_value().left().unwrap(); + let call_result = call_roc_function(env, roc_function, &return_layout, arguments); let return_value = make_good_roc_result(env, call_result); @@ -3738,10 +3727,12 @@ where fn make_exception_catcher<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, roc_function: FunctionValue<'ctx>, + return_layout: Layout<'a>, ) -> FunctionValue<'ctx> { let wrapper_function_name = format!("{}_catcher", roc_function.get_name().to_str().unwrap()); - let function_value = make_exception_catching_wrapper(env, roc_function, &wrapper_function_name); + let function_value = + make_exception_catching_wrapper(env, roc_function, return_layout, &wrapper_function_name); function_value.set_linkage(Linkage::Internal); @@ -3775,6 +3766,7 @@ fn make_good_roc_result<'a, 'ctx, 'env>( fn make_exception_catching_wrapper<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, roc_function: FunctionValue<'ctx>, + return_layout: Layout<'a>, wrapper_function_name: &str, ) -> FunctionValue<'ctx> { // build the C calling convention wrapper @@ -3783,12 +3775,20 @@ fn make_exception_catching_wrapper<'a, 'ctx, 'env>( let builder = env.builder; let roc_function_type = roc_function.get_type(); - let argument_types = roc_function_type.get_param_types(); + let argument_types = match RocReturn::from_layout(env, &return_layout) { + RocReturn::Return => roc_function_type.get_param_types(), + RocReturn::ByPointer => { + let mut types = roc_function_type.get_param_types(); + types.remove(0); + + types + } + }; let wrapper_return_type = context.struct_type( &[ context.i64_type().into(), - roc_function.get_type().get_return_type().unwrap(), + basic_type_from_layout(env, &return_layout), ], false, ); @@ -3825,9 +3825,8 @@ fn make_exception_catching_wrapper<'a, 'ctx, 'env>( env, wrapper_function, roc_function, - roc_function.get_call_conventions(), &arguments, - roc_function_type.get_return_type().unwrap(), + return_layout, ); builder.build_return(Some(&result)); @@ -4029,10 +4028,7 @@ fn build_proc_header<'a, 'ctx, 'env>( let fn_type = match RocReturn::from_layout(env, &proc.ret_layout) { RocReturn::Return => ret_type.fn_type(&arg_basic_types, false), RocReturn::ByPointer => { - println!( - "{:?} will return void instead of {:?}", - symbol, proc.ret_layout - ); + // println!( "{:?} will return void instead of {:?}", symbol, proc.ret_layout); arg_basic_types.push(ret_type.ptr_type(AddressSpace::Generic).into()); env.context.void_type().fn_type(&arg_basic_types, false) } @@ -4141,9 +4137,8 @@ pub fn build_closure_caller<'a, 'ctx, 'env>( env, function_value, evaluator, - evaluator.get_call_conventions(), &evaluator_arguments, - result_type, + *return_layout, ) } else { call_roc_function(env, evaluator, return_layout, &evaluator_arguments) @@ -4404,7 +4399,7 @@ fn roc_call_with_args<'a, 'ctx, 'env>( call_roc_function(env, fn_val, result_layout, arguments) } -fn call_roc_function<'a, 'ctx, 'env>( +pub fn call_roc_function<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, roc_function: FunctionValue<'ctx>, result_layout: &Layout<'a>, @@ -4510,6 +4505,7 @@ fn roc_function_call<'a, 'ctx, 'env>( lambda_set: LambdaSet<'a>, closure_data_is_owned: bool, argument_layouts: &[Layout<'a>], + result_layout: Layout<'a>, ) -> RocFunctionCall<'ctx> { use crate::llvm::bitcode::{build_inc_n_wrapper, build_transform_caller}; @@ -4518,9 +4514,10 @@ fn roc_function_call<'a, 'ctx, 'env>( .build_alloca(closure_data.get_type(), "closure_data_ptr"); env.builder.build_store(closure_data_ptr, closure_data); - let stepper_caller = build_transform_caller(env, transform, lambda_set, argument_layouts) - .as_global_value() - .as_pointer_value(); + let stepper_caller = + build_transform_caller(env, transform, lambda_set, argument_layouts, result_layout) + .as_global_value() + .as_pointer_value(); let inc_closure_data = build_inc_n_wrapper(env, layout_ids, &lambda_set.runtime_representation()) @@ -4593,6 +4590,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + *result_layout, ); crate::llvm::build_list::list_walk_generic( @@ -4634,6 +4632,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + **result_layout, ); list_map(env, roc_function_call, list, element_layout, result_layout) @@ -4663,6 +4662,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + **result_layout, ); list_map2( @@ -4706,6 +4706,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + **result_layout, ); list_map3( @@ -4764,6 +4765,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + **result_layout, ); list_map4( @@ -4810,6 +4812,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + **result_layout, ); list_map_with_index(env, roc_function_call, list, element_layout, result_layout) @@ -4836,6 +4839,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + *result_layout, ); list_keep_if(env, layout_ids, roc_function_call, list, element_layout) @@ -4866,6 +4870,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + *result_layout, ); list_keep_oks( @@ -4906,6 +4911,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + *result_layout, ); list_keep_errs( @@ -4958,6 +4964,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + *result_layout, ); list_sort_with( @@ -4993,6 +5000,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + *result_layout, ); dict_walk( From 51756cbc894eef2455cf9b63c34f3ce5726d8e0b Mon Sep 17 00:00:00 2001 From: Folkert Date: Fri, 5 Nov 2021 11:21:06 +0100 Subject: [PATCH 08/44] fix uninitialized alloca --- compiler/gen_llvm/src/llvm/build.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 242695fa0e..52d18411ed 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -3499,7 +3499,7 @@ fn expose_function_to_host_help_c_abi<'a, 'ctx, 'env>( } } - let arguments_for_call = &arguments_for_call.into_bump_slice(); + let arguments_for_call = arguments_for_call.into_bump_slice(); let call_result = call_roc_function(env, roc_function, &return_layout, arguments_for_call); @@ -4411,11 +4411,12 @@ pub fn call_roc_function<'a, 'ctx, 'env>( RocReturn::ByPointer => { if !pass_by_pointer { let mut arguments = Vec::from_iter_in(arguments.iter().copied(), env.arena); + arguments.pop(); let result_type = basic_type_from_layout(env, result_layout); let result_alloca = env.builder.build_alloca(result_type, "result_value"); - //arguments.push(result_alloca.into()); + arguments.push(result_alloca.into()); debug_assert_eq!( roc_function.get_type().get_param_types().len(), From 5cd232816b9ee4635cd1fa6a9cb7ac076f4271d7 Mon Sep 17 00:00:00 2001 From: Folkert Date: Fri, 5 Nov 2021 21:30:20 +0100 Subject: [PATCH 09/44] waypoint --- cli/src/build.rs | 2 +- compiler/gen_llvm/src/llvm/bitcode.rs | 72 +++--- compiler/gen_llvm/src/llvm/build.rs | 269 +++++++++++++++------- compiler/gen_llvm/src/llvm/convert.rs | 60 +++++ compiler/gen_llvm/src/llvm/refcounting.rs | 33 +-- compiler/mono/src/layout.rs | 50 +++- compiler/test_gen/src/gen_tags.rs | 3 +- compiler/test_gen/src/helpers/eval.rs | 4 + 8 files changed, 337 insertions(+), 156 deletions(-) diff --git a/cli/src/build.rs b/cli/src/build.rs index 39766a9e67..4f500974e9 100644 --- a/cli/src/build.rs +++ b/cli/src/build.rs @@ -255,7 +255,7 @@ pub fn build_file<'a>( link( target, binary_path.clone(), - &inputs, + &inputs, link_type ) .map_err(|_| { diff --git a/compiler/gen_llvm/src/llvm/bitcode.rs b/compiler/gen_llvm/src/llvm/bitcode.rs index 5116f5a033..d006b5b508 100644 --- a/compiler/gen_llvm/src/llvm/bitcode.rs +++ b/compiler/gen_llvm/src/llvm/bitcode.rs @@ -1,7 +1,7 @@ /// Helpers for interacting with the zig that generates bitcode use crate::debug_info_init; use crate::llvm::build::{struct_from_fields, Env, C_CALL_CONV, FAST_CALL_CONV, TAG_DATA_INDEX}; -use crate::llvm::convert::basic_type_from_layout; +use crate::llvm::convert::{basic_type_from_layout, basic_type_from_layout_1}; use crate::llvm::refcounting::{ decrement_refcount_layout, increment_n_refcount_layout, increment_refcount_layout, }; @@ -128,20 +128,19 @@ fn build_has_tag_id_help<'a, 'ctx, 'env>( [tag_id, tag_value_ptr] => { let tag_type = basic_type_from_layout(env, &Layout::Union(union_layout)); - let argument_cast = env - .builder - .build_bitcast( - *tag_value_ptr, - tag_type.ptr_type(AddressSpace::Generic), - "load_opaque", - ) - .into_pointer_value(); - - let tag_value = env.builder.build_load(argument_cast, "get_value"); + let tag_value = env.builder.build_pointer_cast( + tag_value_ptr.into_pointer_value(), + tag_type.ptr_type(AddressSpace::Generic), + "load_opaque_get_tag_id", + ); let actual_tag_id = { - let tag_id_i64 = - crate::llvm::build::get_tag_id(env, function_value, &union_layout, tag_value); + let tag_id_i64 = crate::llvm::build::get_tag_id( + env, + function_value, + &union_layout, + tag_value.into(), + ); env.builder .build_int_cast(tag_id_i64, env.context.i16_type(), "to_i16") @@ -263,12 +262,22 @@ fn build_transform_caller_help<'a, 'ctx, 'env>( for (argument_ptr, layout) in arguments.iter().zip(argument_layouts) { let basic_type = basic_type_from_layout(env, layout).ptr_type(AddressSpace::Generic); - let argument_cast = env - .builder - .build_bitcast(*argument_ptr, basic_type, "load_opaque") - .into_pointer_value(); + let argument = if layout.is_passed_by_reference() { + env.builder + .build_pointer_cast( + argument_ptr.into_pointer_value(), + basic_type, + "cast_ptr_to_tag", + ) + .into() + } else { + let argument_cast = env + .builder + .build_bitcast(*argument_ptr, basic_type, "load_opaque_1") + .into_pointer_value(); - let argument = env.builder.build_load(argument_cast, "load_opaque"); + env.builder.build_load(argument_cast, "load_opaque_2") + }; arguments_cast.push(argument); } @@ -300,17 +309,10 @@ fn build_transform_caller_help<'a, 'ctx, 'env>( let result_u8_ptr = function_value .get_nth_param(argument_layouts.len() as u32 + 1) - .unwrap(); - let result_ptr = env - .builder - .build_bitcast( - result_u8_ptr, - result.get_type().ptr_type(AddressSpace::Generic), - "write_result", - ) + .unwrap() .into_pointer_value(); - env.builder.build_store(result_ptr, result); + crate::llvm::build::store_roc_value_opaque(env, result_layout, result_u8_ptr, result); env.builder.build_return(None); env.builder.position_at_end(block); @@ -412,12 +414,18 @@ fn build_rc_wrapper<'a, 'ctx, 'env>( let value_type = basic_type_from_layout(env, layout).ptr_type(AddressSpace::Generic); - let value_cast = env - .builder - .build_bitcast(value_ptr, value_type, "load_opaque") - .into_pointer_value(); + let value = if layout.is_passed_by_reference() { + env.builder + .build_pointer_cast(value_ptr, value_type, "cast_ptr_to_tag") + .into() + } else { + let value_cast = env + .builder + .build_bitcast(value_ptr, value_type, "load_opaque") + .into_pointer_value(); - let value = env.builder.build_load(value_cast, "load_opaque"); + env.builder.build_load(value_cast, "load_opaque") + }; match rc_operation { Mode::Inc => { diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 52d18411ed..a6fe38761a 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -21,7 +21,8 @@ use crate::llvm::build_str::{ }; use crate::llvm::compare::{generic_eq, generic_neq}; use crate::llvm::convert::{ - basic_type_from_builtin, basic_type_from_layout, block_of_memory_slices, ptr_int, + basic_type_from_builtin, basic_type_from_layout, basic_type_from_layout_1, + block_of_memory_slices, ptr_int, }; use crate::llvm::refcounting::{ build_reset, decrement_refcount_layout, increment_refcount_layout, PointerToRefcount, @@ -618,8 +619,8 @@ pub fn construct_optimization_passes<'a>( mpm.add_always_inliner_pass(); // tail-call elimination is always on - fpm.add_instruction_combining_pass(); - fpm.add_tail_call_elimination_pass(); + // fpm.add_instruction_combining_pass(); + // fpm.add_tail_call_elimination_pass(); let pmb = PassManagerBuilder::create(); match opt_level { @@ -1235,32 +1236,21 @@ pub fn build_exp_expr<'a, 'ctx, 'env>( match union_layout { UnionLayout::NonRecursive(tag_layouts) => { - debug_assert!(argument.is_struct_value()); + debug_assert!(argument.is_pointer_value()); + let field_layouts = tag_layouts[*tag_id as usize]; - let struct_layout = Layout::Struct(field_layouts); - let struct_type = basic_type_from_layout(env, &struct_layout); + let tag_id_type = + basic_type_from_layout(env, &union_layout.tag_id_layout()).into_int_type(); - let argument_pointer = builder.build_alloca(argument.get_type(), "cast_alloca"); - builder.build_store(argument_pointer, argument); - - let struct_ptr = builder.build_pointer_cast( - argument_pointer, - struct_type.ptr_type(AddressSpace::Generic), - "foobar", - ); - - // let struct_value = access_index_struct_value( - // builder, - // argument.into_struct_value(), - // struct_type.into_struct_type(), - // ); - - let result = builder - .build_struct_gep(struct_ptr, *index as u32, "") - .expect("desired field did not decode"); - - builder.build_load(result, "foobarbaz") + lookup_at_index_ptr2( + env, + union_layout, + tag_id_type, + field_layouts, + *index as usize, + argument.into_pointer_value(), + ) } UnionLayout::Recursive(tag_layouts) => { debug_assert!(argument.is_pointer_value()); @@ -1555,10 +1545,13 @@ pub fn build_tag<'a, 'ctx, 'env>( .builder .build_struct_gep(struct_ptr, index, "get_tag_field_ptr") .unwrap(); - env.builder.build_store(ptr, field_val); + + let field_layout = tag_field_layouts[index as usize]; + store_roc_value(env, field_layout, ptr, field_val); } - env.builder.build_load(result_alloca, "load_result") + // env.builder.build_load(result_alloca, "load_result") + result_alloca.into() } UnionLayout::Recursive(tags) => { debug_assert!(union_size > 1); @@ -1866,9 +1859,10 @@ pub fn get_tag_id<'a, 'ctx, 'env>( match union_layout { UnionLayout::NonRecursive(_) => { - let tag = argument.into_struct_value(); + debug_assert!(argument.is_pointer_value(), "{:?}", argument); - get_tag_id_non_recursive(env, tag) + let argument_ptr = argument.into_pointer_value(); + get_tag_id_wrapped(env, argument_ptr) } UnionLayout::Recursive(_) => { let argument_ptr = argument.into_pointer_value(); @@ -2008,7 +2002,26 @@ fn lookup_at_index_ptr2<'a, 'ctx, 'env>( .build_struct_gep(data_ptr, index as u32, "at_index_struct_gep") .unwrap(); - let result = builder.build_load(elem_ptr, "load_at_index_ptr"); + let field_layout = field_layouts[index]; + let result = if field_layout.is_passed_by_reference() { + let field_type = basic_type_from_layout(env, &field_layout); + + let align_bytes = field_layout.alignment_bytes(env.ptr_bytes); + let alloca = env.builder.build_alloca(field_type, "copied_tag"); + if align_bytes > 0 { + let size = env + .ptr_int() + .const_int(field_layout.stack_size(env.ptr_bytes) as u64, false); + + env.builder + .build_memcpy(alloca, align_bytes, elem_ptr, align_bytes, size) + .unwrap(); + } + + alloca.into() + } else { + builder.build_load(elem_ptr, "load_at_index_ptr") + }; if let Some(Layout::RecursivePointer) = field_layouts.get(index as usize) { // a recursive field is stored as a `i64*`, to use it we must cast it to @@ -2155,17 +2168,12 @@ pub fn allocate_with_refcount_help<'a, 'ctx, 'env>( let ptr_type = value_type.ptr_type(AddressSpace::Generic); unsafe { - builder - .build_bitcast( - env.builder.build_in_bounds_gep( - as_usize_ptr, - &[index_intvalue], - "get_data_ptr", - ), - ptr_type, - "alloc_cast_to_desired", - ) - .into_pointer_value() + builder.build_pointer_cast( + env.builder + .build_in_bounds_gep(as_usize_ptr, &[index_intvalue], "get_data_ptr"), + ptr_type, + "alloc_cast_to_desired", + ) } }; @@ -2191,13 +2199,13 @@ pub fn allocate_with_refcount_help<'a, 'ctx, 'env>( fn list_literal<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, scope: &Scope<'a, 'ctx>, - elem_layout: &Layout<'a>, + element_layout: &Layout<'a>, elems: &[ListLiteralElement], ) -> BasicValueEnum<'ctx> { let ctx = env.context; let builder = env.builder; - let element_type = basic_type_from_layout(env, elem_layout); + let element_type = basic_type_from_layout(env, element_layout); let list_length = elems.len(); let list_length_intval = env.ptr_int().const_int(list_length as _, false); @@ -2207,9 +2215,9 @@ fn list_literal<'a, 'ctx, 'env>( // if element_type.is_int_type() { if false { let element_type = element_type.into_int_type(); - let element_width = elem_layout.stack_size(env.ptr_bytes); + let element_width = element_layout.stack_size(env.ptr_bytes); let size = list_length * element_width as usize; - let alignment = elem_layout + let alignment = element_layout .alignment_bytes(env.ptr_bytes) .max(env.ptr_bytes); @@ -2241,7 +2249,7 @@ fn list_literal<'a, 'ctx, 'env>( for (index, element) in elems.iter().enumerate() { match element { ListLiteralElement::Literal(literal) => { - let val = build_exp_literal(env, elem_layout, literal); + let val = build_exp_literal(env, element_layout, literal); global_elements.push(val.into_int_value()); } ListLiteralElement::Symbol(symbol) => { @@ -2296,7 +2304,7 @@ fn list_literal<'a, 'ctx, 'env>( super::build_list::store_list(env, ptr, list_length_intval) } else { // some of our elements are non-constant, so we must allocate space on the heap - let ptr = allocate_list(env, elem_layout, list_length_intval); + let ptr = allocate_list(env, element_layout, list_length_intval); // then, copy the relevant segment from the constant section into the heap env.builder @@ -2320,26 +2328,69 @@ fn list_literal<'a, 'ctx, 'env>( super::build_list::store_list(env, ptr, list_length_intval) } } else { - let ptr = allocate_list(env, elem_layout, list_length_intval); + let ptr = allocate_list(env, element_layout, list_length_intval); // Copy the elements from the list literal into the array for (index, element) in elems.iter().enumerate() { let val = match element { ListLiteralElement::Literal(literal) => { - build_exp_literal(env, elem_layout, literal) + build_exp_literal(env, element_layout, literal) } ListLiteralElement::Symbol(symbol) => load_symbol(scope, symbol), }; let index_val = ctx.i64_type().const_int(index as u64, false); let elem_ptr = unsafe { builder.build_in_bounds_gep(ptr, &[index_val], "index") }; - builder.build_store(elem_ptr, val); + store_roc_value(env, *element_layout, elem_ptr, val); } super::build_list::store_list(env, ptr, list_length_intval) } } +pub fn store_roc_value_opaque<'a, 'ctx, 'env>( + env: &Env<'a, 'ctx, 'env>, + layout: Layout<'a>, + opaque_destination: PointerValue<'ctx>, + value: BasicValueEnum<'ctx>, +) { + let target_type = basic_type_from_layout(env, &layout).ptr_type(AddressSpace::Generic); + let destination = + env.builder + .build_pointer_cast(opaque_destination, target_type, "store_roc_value_opaque"); + + store_roc_value(env, layout, destination, value) +} + +pub fn store_roc_value<'a, 'ctx, 'env>( + env: &Env<'a, 'ctx, 'env>, + layout: Layout<'a>, + destination: PointerValue<'ctx>, + value: BasicValueEnum<'ctx>, +) { + if layout.is_passed_by_reference() { + let align_bytes = layout.alignment_bytes(env.ptr_bytes); + + if align_bytes > 0 { + let size = env + .ptr_int() + .const_int(layout.stack_size(env.ptr_bytes) as u64, false); + + env.builder + .build_memcpy( + destination, + align_bytes, + value.into_pointer_value(), + align_bytes, + size, + ) + .unwrap(); + } + } else { + env.builder.build_store(destination, value); + } +} + pub fn build_exp_stmt<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, layout_ids: &mut LayoutIds<'a>, @@ -2415,8 +2466,7 @@ pub fn build_exp_stmt<'a, 'ctx, 'env>( let out_parameter = parameters.last().unwrap(); debug_assert!(out_parameter.is_pointer_value()); - env.builder - .build_store(out_parameter.into_pointer_value(), value); + store_roc_value(env, *layout, out_parameter.into_pointer_value(), value); if let Some(block) = env.builder.get_insert_block() { match block.get_terminator() { @@ -3656,7 +3706,7 @@ fn set_jump_and_catch_long_jump<'a, 'ctx, 'env>( let call_result = call_roc_function(env, roc_function, &return_layout, arguments); - let return_value = make_good_roc_result(env, call_result); + let return_value = make_good_roc_result(env, return_layout, call_result); builder.build_store(result_alloca, return_value); @@ -3721,7 +3771,7 @@ fn set_jump_and_catch_long_jump<'a, 'ctx, 'env>( env.builder.position_at_end(cont_block); - builder.build_load(result_alloca, "load_result") + builder.build_load(result_alloca, "set_jump_and_catch_long_jump_load_result") } fn make_exception_catcher<'a, 'ctx, 'env>( @@ -3741,12 +3791,13 @@ fn make_exception_catcher<'a, 'ctx, 'env>( fn make_good_roc_result<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, + return_layout: Layout<'a>, return_value: BasicValueEnum<'ctx>, ) -> BasicValueEnum<'ctx> { let context = env.context; let builder = env.builder; - let content_type = return_value.get_type(); + let content_type = basic_type_from_layout(env, &return_layout); let wrapper_return_type = context.struct_type(&[context.i64_type().into(), content_type], false); @@ -3756,9 +3807,19 @@ fn make_good_roc_result<'a, 'ctx, 'env>( .build_insert_value(v1, context.i64_type().const_zero(), 0, "set_no_error") .unwrap(); - let v3 = builder - .build_insert_value(v2, return_value, 1, "set_call_result") - .unwrap(); + let v3 = if return_layout.is_passed_by_reference() { + let loaded = env.builder.build_load( + return_value.into_pointer_value(), + "load_call_result_passed_by_ptr", + ); + builder + .build_insert_value(v2, loaded, 1, "set_call_result") + .unwrap() + } else { + builder + .build_insert_value(v2, return_value, 1, "set_call_result") + .unwrap() + }; v3.into_struct_value().into() } @@ -3971,6 +4032,8 @@ fn build_procedures_help<'a, 'ctx, 'env>( app_ll_file, ); } else { + env.module.print_to_stderr(); + panic!( "The preceding code was from {:?}, which failed LLVM verification in {} build.", fn_val.get_name().to_str().unwrap(), @@ -4020,7 +4083,7 @@ fn build_proc_header<'a, 'ctx, 'env>( let mut arg_basic_types = Vec::with_capacity_in(args.len(), arena); for (layout, _) in args.iter() { - let arg_type = basic_type_from_layout(env, layout); + let arg_type = basic_type_from_layout_1(env, layout); arg_basic_types.push(arg_type); } @@ -4132,19 +4195,41 @@ pub fn build_closure_caller<'a, 'ctx, 'env>( } } - let call_result = if env.is_gen_test { - set_jump_and_catch_long_jump( + if env.is_gen_test { + let call_result = set_jump_and_catch_long_jump( env, function_value, evaluator, &evaluator_arguments, *return_layout, - ) - } else { - call_roc_function(env, evaluator, return_layout, &evaluator_arguments) - }; + ); - builder.build_store(output, call_result); + builder.build_store(output, call_result); + } else { + let call_result = call_roc_function(env, evaluator, return_layout, &evaluator_arguments); + + if return_layout.is_passed_by_reference() { + let align_bytes = return_layout.alignment_bytes(env.ptr_bytes); + + if align_bytes > 0 { + let size = env + .ptr_int() + .const_int(return_layout.stack_size(env.ptr_bytes) as u64, false); + + env.builder + .build_memcpy( + output, + align_bytes, + call_result.into_pointer_value(), + align_bytes, + size, + ) + .unwrap(); + } + } else { + builder.build_store(output, call_result); + } + }; builder.build_return(None); @@ -4408,29 +4493,10 @@ pub fn call_roc_function<'a, 'ctx, 'env>( let pass_by_pointer = roc_function.get_type().get_param_types().len() == arguments.len() + 1; match RocReturn::from_layout(env, result_layout) { - RocReturn::ByPointer => { - if !pass_by_pointer { - let mut arguments = Vec::from_iter_in(arguments.iter().copied(), env.arena); - arguments.pop(); - - let result_type = basic_type_from_layout(env, result_layout); - let result_alloca = env.builder.build_alloca(result_type, "result_value"); - - arguments.push(result_alloca.into()); - - debug_assert_eq!( - roc_function.get_type().get_param_types().len(), - arguments.len() - ); - let call = env.builder.build_call(roc_function, &arguments, "call"); - - // roc functions should have the fast calling convention - debug_assert_eq!(roc_function.get_call_conventions(), FAST_CALL_CONV); - call.set_call_convention(FAST_CALL_CONV); - - return env.builder.build_load(result_alloca, "load_result"); - } + RocReturn::ByPointer if !pass_by_pointer => { + // WARNING this is a hack!! let mut arguments = Vec::from_iter_in(arguments.iter().copied(), env.arena); + arguments.pop(); let result_type = basic_type_from_layout(env, result_layout); let result_alloca = env.builder.build_alloca(result_type, "result_value"); @@ -4449,6 +4515,31 @@ pub fn call_roc_function<'a, 'ctx, 'env>( env.builder.build_load(result_alloca, "load_result") } + RocReturn::ByPointer => { + let mut arguments = Vec::from_iter_in(arguments.iter().copied(), env.arena); + + let result_type = basic_type_from_layout(env, result_layout); + let result_alloca = env.builder.build_alloca(result_type, "result_value"); + + arguments.push(result_alloca.into()); + + debug_assert_eq!( + roc_function.get_type().get_param_types().len(), + arguments.len() + ); + let call = env.builder.build_call(roc_function, &arguments, "call"); + + // roc functions should have the fast calling convention + debug_assert_eq!(roc_function.get_call_conventions(), FAST_CALL_CONV); + call.set_call_convention(FAST_CALL_CONV); + + if result_layout.is_passed_by_reference() { + result_alloca.into() + } else { + env.builder + .build_load(result_alloca, "return_by_pointer_load_result") + } + } RocReturn::Return => { debug_assert_eq!( roc_function.get_type().get_param_types().len(), diff --git a/compiler/gen_llvm/src/llvm/convert.rs b/compiler/gen_llvm/src/llvm/convert.rs index d32216c10a..559945b083 100644 --- a/compiler/gen_llvm/src/llvm/convert.rs +++ b/compiler/gen_llvm/src/llvm/convert.rs @@ -76,6 +76,66 @@ pub fn basic_type_from_layout<'a, 'ctx, 'env>( } } +pub fn basic_type_from_layout_1<'a, 'ctx, 'env>( + env: &crate::llvm::build::Env<'a, 'ctx, 'env>, + layout: &Layout<'_>, +) -> BasicTypeEnum<'ctx> { + use Layout::*; + + match layout { + Struct(sorted_fields) => basic_type_from_record(env, sorted_fields), + LambdaSet(lambda_set) => { + basic_type_from_layout_1(env, &lambda_set.runtime_representation()) + } + Union(union_layout) => { + use UnionLayout::*; + + let tag_id_type = basic_type_from_layout_1(env, &union_layout.tag_id_layout()); + + match union_layout { + NonRecursive(tags) => { + let data = block_of_memory_slices(env.context, tags, env.ptr_bytes); + let struct_type = env.context.struct_type(&[data, tag_id_type], false); + + struct_type.ptr_type(AddressSpace::Generic).into() + } + Recursive(tags) + | NullableWrapped { + other_tags: tags, .. + } => { + let data = block_of_memory_slices(env.context, tags, env.ptr_bytes); + + if union_layout.stores_tag_id_as_data(env.ptr_bytes) { + env.context + .struct_type(&[data, tag_id_type], false) + .ptr_type(AddressSpace::Generic) + .into() + } else { + data.ptr_type(AddressSpace::Generic).into() + } + } + NullableUnwrapped { other_fields, .. } => { + let block = block_of_memory_slices(env.context, &[other_fields], env.ptr_bytes); + block.ptr_type(AddressSpace::Generic).into() + } + NonNullableUnwrapped(fields) => { + let block = block_of_memory_slices(env.context, &[fields], env.ptr_bytes); + block.ptr_type(AddressSpace::Generic).into() + } + } + } + RecursivePointer => { + // TODO make this dynamic + env.context + .i64_type() + .ptr_type(AddressSpace::Generic) + .as_basic_type_enum() + } + + Builtin(builtin) => basic_type_from_builtin(env, builtin), + } +} + pub fn basic_type_from_builtin<'a, 'ctx, 'env>( env: &crate::llvm::build::Env<'a, 'ctx, 'env>, builtin: &Builtin<'_>, diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index f183e029f6..5944f19e25 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -1,11 +1,11 @@ use crate::debug_info_init; use crate::llvm::bitcode::call_void_bitcode_fn; use crate::llvm::build::{ - add_func, cast_basic_basic, cast_block_of_memory_to_tag, get_tag_id, get_tag_id_non_recursive, - tag_pointer_clear_tag_id, Env, FAST_CALL_CONV, TAG_DATA_INDEX, TAG_ID_INDEX, + add_func, cast_basic_basic, get_tag_id, tag_pointer_clear_tag_id, Env, FAST_CALL_CONV, + TAG_DATA_INDEX, TAG_ID_INDEX, }; use crate::llvm::build_list::{incrementing_elem_loop, list_len, load_list}; -use crate::llvm::convert::{basic_type_from_layout, ptr_int}; +use crate::llvm::convert::{basic_type_from_layout, basic_type_from_layout_1, ptr_int}; use bumpalo::collections::Vec; use inkwell::basic_block::BasicBlock; use inkwell::context::Context; @@ -542,24 +542,6 @@ fn modify_refcount_layout_help<'a, 'ctx, 'env>( } }, _ => { - if let Layout::Union(UnionLayout::NonRecursive(_)) = layout { - let alloca = env.builder.build_alloca(value.get_type(), "tag_union"); - env.builder.build_store(alloca, value); - call_help(env, function, call_mode, alloca.into()); - return; - } - - if let Layout::LambdaSet(lambda_set) = layout { - if let Layout::Union(UnionLayout::NonRecursive(_)) = - lambda_set.runtime_representation() - { - let alloca = env.builder.build_alloca(value.get_type(), "tag_union"); - env.builder.build_store(alloca, value); - call_help(env, function, call_mode, alloca.into()); - return; - } - } - call_help(env, function, call_mode, value); } } @@ -1620,10 +1602,9 @@ fn modify_refcount_union<'a, 'ctx, 'env>( let function = match env.module.get_function(fn_name.as_str()) { Some(function_value) => function_value, None => { - let basic_type = basic_type_from_layout(env, &layout); - // we pass tag unions by reference for efficiency - let ptr_type = basic_type.ptr_type(AddressSpace::Generic).into(); - let function_value = build_header(env, ptr_type, mode, &fn_name); + let basic_type = basic_type_from_layout_1(env, &layout); + let function_value = build_header(env, basic_type, mode, &fn_name); + dbg!(function_value); modify_refcount_union_help( env, @@ -1675,6 +1656,8 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( let before_block = env.builder.get_insert_block().expect("to be in a function"); + dbg!(fn_val, arg_ptr); + // read the tag_id let tag_id_ptr = env .builder diff --git a/compiler/mono/src/layout.rs b/compiler/mono/src/layout.rs index 2c63b7658d..cf30803e3b 100644 --- a/compiler/mono/src/layout.rs +++ b/compiler/mono/src/layout.rs @@ -863,6 +863,16 @@ impl<'a> Layout<'a> { false } + pub fn is_passed_by_reference(&self) -> bool { + match self { + Layout::Union(UnionLayout::NonRecursive(_)) => true, + Layout::LambdaSet(lambda_set) => { + lambda_set.runtime_representation().is_passed_by_reference() + } + _ => false, + } + } + pub fn stack_size(&self, pointer_size: u32) -> u32 { let width = self.stack_size_without_alignment(pointer_size); let alignment = self.alignment_bytes(pointer_size); @@ -941,16 +951,16 @@ impl<'a> Layout<'a> { }) .max(); + let tag_id_builtin = variant.tag_id_builtin(); match max_alignment { - Some(align) => { - let tag_id_builtin = variant.tag_id_builtin(); - - round_up_to_alignment( - align, - tag_id_builtin.alignment_bytes(pointer_size), - ) + Some(align) => round_up_to_alignment( + align.max(tag_id_builtin.alignment_bytes(pointer_size)), + tag_id_builtin.alignment_bytes(pointer_size), + ), + None => { + // none of the tags had any payload, but the tag id still contains information + tag_id_builtin.alignment_bytes(pointer_size) } - None => 0, } } Recursive(_) @@ -2675,3 +2685,27 @@ impl<'a> std::convert::TryFrom<&Layout<'a>> for ListLayout<'a> { } } } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn width_and_alignment_union_empty_struct() { + let lambda_set = LambdaSet { + set: &[(Symbol::LIST_MAP, &[])], + representation: &Layout::Struct(&[]), + }; + + let a = &[Layout::Struct(&[])] as &[_]; + let b = &[Layout::LambdaSet(lambda_set)] as &[_]; + let tt = [a, b]; + + let layout = Layout::Union(UnionLayout::NonRecursive(&tt)); + + // at the moment, the tag id uses an I64, so + let ptr_width = 8; + assert_eq!(layout.stack_size(ptr_width), 8); + assert_eq!(layout.alignment_bytes(ptr_width), 8); + } +} diff --git a/compiler/test_gen/src/gen_tags.rs b/compiler/test_gen/src/gen_tags.rs index fe50a16cde..9c5989b174 100644 --- a/compiler/test_gen/src/gen_tags.rs +++ b/compiler/test_gen/src/gen_tags.rs @@ -3,6 +3,7 @@ use crate::assert_evals_to; // use crate::assert_wasm_evals_to as assert_evals_to; use indoc::indoc; +use roc_mono::layout::LambdaSet; use roc_std::{RocList, RocStr}; #[test] @@ -423,7 +424,7 @@ fn result_with_guard_pattern() { } #[test] -fn maybe_is_just() { +fn maybe_is_just_not_nested() { assert_evals_to!( indoc!( r#" diff --git a/compiler/test_gen/src/helpers/eval.rs b/compiler/test_gen/src/helpers/eval.rs index e844d420ea..f6e4bffad2 100644 --- a/compiler/test_gen/src/helpers/eval.rs +++ b/compiler/test_gen/src/helpers/eval.rs @@ -198,6 +198,10 @@ fn create_llvm_module<'a>( if name.starts_with("roc_builtins.dict") { function.add_attribute(AttributeLoc::Function, attr); } + + if name.starts_with("roc_builtins.list") { + function.add_attribute(AttributeLoc::Function, attr); + } } // Compile and add all the Procs before adding main From bd0f02c542343b3454fb52137ef2c1ea4c975a8d Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 6 Nov 2021 19:27:16 +0100 Subject: [PATCH 10/44] another waypoint --- compiler/gen_llvm/src/llvm/build.rs | 41 ++++++++++++++--- compiler/gen_llvm/src/llvm/build_hash.rs | 29 ++++-------- compiler/gen_llvm/src/llvm/compare.rs | 55 +++++++++++------------ compiler/gen_llvm/src/llvm/convert.rs | 2 +- compiler/gen_llvm/src/llvm/refcounting.rs | 18 ++++++-- 5 files changed, 84 insertions(+), 61 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index a6fe38761a..83d6d8d253 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -1065,7 +1065,16 @@ pub fn build_exp_expr<'a, 'ctx, 'env>( if !field_layout.is_dropped_because_empty() { field_types.push(basic_type_from_layout(env, field_layout)); - field_vals.push(field_expr); + if field_layout.is_passed_by_reference() { + let field_value = env.builder.build_load( + field_expr.into_pointer_value(), + "load_tag_to_put_in_struct", + ); + + field_vals.push(field_value); + } else { + field_vals.push(field_expr); + } } } @@ -1173,14 +1182,29 @@ pub fn build_exp_expr<'a, 'ctx, 'env>( match (value, layout) { (StructValue(argument), Layout::Struct(fields)) => { debug_assert!(!fields.is_empty()); - env.builder + + let field_value = env + .builder .build_extract_value( argument, *index as u32, env.arena .alloc(format!("struct_field_access_record_{}", index)), ) - .unwrap() + .unwrap(); + + let field_layout = fields[*index as usize]; + if field_layout.is_passed_by_reference() { + let alloca = env.builder.build_alloca( + basic_type_from_layout(env, &field_layout), + "struct_field_tag", + ); + env.builder.build_store(alloca, field_value); + + alloca.into() + } else { + field_value + } } ( PointerValue(argument), @@ -2555,7 +2579,11 @@ pub fn build_exp_stmt<'a, 'ctx, 'env>( builder.position_at_end(cont_block); for (ptr, param) in joinpoint_args.iter().zip(parameters.iter()) { - let value = env.builder.build_load(*ptr, "load_jp_argument"); + let value = if param.layout.is_passed_by_reference() { + (*ptr).into() + } else { + env.builder.build_load(*ptr, "load_jp_argument") + }; scope.insert(param.symbol, (param.layout, value)); } @@ -2582,8 +2610,9 @@ pub fn build_exp_stmt<'a, 'ctx, 'env>( let (cont_block, argument_pointers) = scope.join_points.get(join_point).unwrap(); for (pointer, argument) in argument_pointers.iter().zip(arguments.iter()) { - let value = load_symbol(scope, argument); - builder.build_store(*pointer, value); + let (value, layout) = load_symbol_and_layout(scope, argument); + + store_roc_value(env, *layout, *pointer, value); } builder.build_unconditional_branch(*cont_block); diff --git a/compiler/gen_llvm/src/llvm/build_hash.rs b/compiler/gen_llvm/src/llvm/build_hash.rs index d3dc006198..7064cc100d 100644 --- a/compiler/gen_llvm/src/llvm/build_hash.rs +++ b/compiler/gen_llvm/src/llvm/build_hash.rs @@ -4,7 +4,7 @@ use crate::llvm::build::tag_pointer_clear_tag_id; use crate::llvm::build::Env; use crate::llvm::build::{cast_block_of_memory_to_tag, get_tag_id, FAST_CALL_CONV, TAG_DATA_INDEX}; use crate::llvm::build_str; -use crate::llvm::convert::basic_type_from_layout; +use crate::llvm::convert::{basic_type_from_layout, basic_type_from_layout_1}; use bumpalo::collections::Vec; use inkwell::values::{ BasicValue, BasicValueEnum, FunctionValue, IntValue, PointerValue, StructValue, @@ -339,7 +339,8 @@ fn build_hash_tag<'a, 'ctx, 'env>( None => { let seed_type = env.context.i64_type(); - let arg_type = basic_type_from_layout(env, layout); + let arg_type = basic_type_from_layout_1(env, layout); + dbg!(layout, arg_type); let function_value = crate::llvm::refcounting::build_header_help( env, @@ -423,14 +424,6 @@ fn hash_tag<'a, 'ctx, 'env>( let block = env.context.append_basic_block(parent, "tag_id_modify"); env.builder.position_at_end(block); - let struct_layout = Layout::Struct(field_layouts); - - let wrapper_type = basic_type_from_layout(env, &struct_layout); - debug_assert!(wrapper_type.is_struct_type()); - - let as_struct = - cast_block_of_memory_to_tag(env.builder, tag.into_struct_value(), wrapper_type); - // hash the tag id let hash_bytes = store_and_use_as_u8_ptr( env, @@ -440,7 +433,6 @@ fn hash_tag<'a, 'ctx, 'env>( .into(), &tag_id_layout, ); - let seed = hash_bitcode_fn( env, seed, @@ -449,14 +441,9 @@ fn hash_tag<'a, 'ctx, 'env>( ); // hash the tag data - let answer = build_hash_struct( - env, - layout_ids, - field_layouts, - WhenRecursive::Unreachable, - seed, - as_struct, - ); + let tag = tag.into_pointer_value(); + let answer = + hash_ptr_to_struct(env, layout_ids, union_layout, field_layouts, seed, tag); merge_phi.add_incoming(&[(&answer, block)]); env.builder.build_unconditional_branch(merge_block); @@ -822,12 +809,12 @@ fn hash_ptr_to_struct<'a, 'ctx, 'env>( ) -> IntValue<'ctx> { use inkwell::types::BasicType; - let wrapper_type = basic_type_from_layout(env, &Layout::Union(*union_layout)); + let wrapper_type = basic_type_from_layout_1(env, &Layout::Union(*union_layout)); // cast the opaque pointer to a pointer of the correct shape let wrapper_ptr = env .builder - .build_bitcast(tag, wrapper_type, "opaque_to_correct") + .build_bitcast(tag, wrapper_type, "hash_ptr_to_struct_opaque_to_correct") .into_pointer_value(); let struct_ptr = env diff --git a/compiler/gen_llvm/src/llvm/compare.rs b/compiler/gen_llvm/src/llvm/compare.rs index 1ca6178c0b..83c8887ce8 100644 --- a/compiler/gen_llvm/src/llvm/compare.rs +++ b/compiler/gen_llvm/src/llvm/compare.rs @@ -1,10 +1,8 @@ use crate::llvm::bitcode::call_bitcode_fn; -use crate::llvm::build::{ - cast_block_of_memory_to_tag, get_tag_id, tag_pointer_clear_tag_id, Env, FAST_CALL_CONV, -}; +use crate::llvm::build::{get_tag_id, tag_pointer_clear_tag_id, Env, FAST_CALL_CONV}; use crate::llvm::build_list::{list_len, load_list_ptr}; use crate::llvm::build_str::str_equal; -use crate::llvm::convert::basic_type_from_layout; +use crate::llvm::convert::{basic_type_from_layout, basic_type_from_layout_1}; use bumpalo::collections::Vec; use inkwell::types::BasicType; use inkwell::values::{ @@ -751,7 +749,7 @@ fn build_tag_eq<'a, 'ctx, 'env>( let function = match env.module.get_function(fn_name.as_str()) { Some(function_value) => function_value, None => { - let arg_type = basic_type_from_layout(env, tag_layout); + let arg_type = basic_type_from_layout_1(env, tag_layout); let function_value = crate::llvm::refcounting::build_header_help( env, @@ -844,9 +842,29 @@ fn build_tag_eq_help<'a, 'ctx, 'env>( match union_layout { NonRecursive(tags) => { + let ptr_equal = env.builder.build_int_compare( + IntPredicate::EQ, + env.builder + .build_ptr_to_int(tag1.into_pointer_value(), env.ptr_int(), "pti"), + env.builder + .build_ptr_to_int(tag2.into_pointer_value(), env.ptr_int(), "pti"), + "compare_pointers", + ); + + let compare_tag_ids = ctx.append_basic_block(parent, "compare_tag_ids"); + + env.builder + .build_conditional_branch(ptr_equal, return_true, compare_tag_ids); + + env.builder.position_at_end(compare_tag_ids); + let id1 = get_tag_id(env, parent, union_layout, tag1); let id2 = get_tag_id(env, parent, union_layout, tag2); + // clear the tag_id so we get a pointer to the actual data + let tag1 = tag1.into_pointer_value(); + let tag2 = tag2.into_pointer_value(); + let compare_tag_fields = ctx.append_basic_block(parent, "compare_tag_fields"); let same_tag = @@ -866,31 +884,8 @@ fn build_tag_eq_help<'a, 'ctx, 'env>( let block = env.context.append_basic_block(parent, "tag_id_modify"); env.builder.position_at_end(block); - // TODO drop tag id? - let struct_layout = Layout::Struct(field_layouts); - - let wrapper_type = basic_type_from_layout(env, &struct_layout); - debug_assert!(wrapper_type.is_struct_type()); - - let struct1 = cast_block_of_memory_to_tag( - env.builder, - tag1.into_struct_value(), - wrapper_type, - ); - let struct2 = cast_block_of_memory_to_tag( - env.builder, - tag2.into_struct_value(), - wrapper_type, - ); - - let answer = build_struct_eq( - env, - layout_ids, - field_layouts, - when_recursive.clone(), - struct1, - struct2, - ); + let answer = + eq_ptr_to_struct(env, layout_ids, union_layout, field_layouts, tag1, tag2); env.builder.build_return(Some(&answer)); diff --git a/compiler/gen_llvm/src/llvm/convert.rs b/compiler/gen_llvm/src/llvm/convert.rs index 559945b083..98306210ad 100644 --- a/compiler/gen_llvm/src/llvm/convert.rs +++ b/compiler/gen_llvm/src/llvm/convert.rs @@ -90,7 +90,7 @@ pub fn basic_type_from_layout_1<'a, 'ctx, 'env>( Union(union_layout) => { use UnionLayout::*; - let tag_id_type = basic_type_from_layout_1(env, &union_layout.tag_id_layout()); + let tag_id_type = basic_type_from_layout(env, &union_layout.tag_id_layout()); match union_layout { NonRecursive(tags) => { diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index 5944f19e25..30c309f540 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -349,18 +349,30 @@ fn modify_refcount_struct_help<'a, 'ctx, 'env>( for (i, field_layout) in layouts.iter().enumerate() { if field_layout.contains_refcounted() { - let field_ptr = env + let raw_value = env .builder .build_extract_value(wrapper_struct, i as u32, "decrement_struct_field") .unwrap(); + let field_value = if field_layout.is_passed_by_reference() { + let field_type = basic_type_from_layout(env, field_layout); + let alloca = env + .builder + .build_alloca(field_type, "load_struct_tag_field_for_decrement"); + env.builder.build_store(alloca, raw_value); + + alloca.into() + } else { + raw_value + }; + modify_refcount_layout_help( env, parent, layout_ids, mode.to_call_mode(fn_val), when_recursive, - field_ptr, + field_value, field_layout, ); } @@ -1289,7 +1301,7 @@ fn build_rec_union_recursive_decrement<'a, 'ctx, 'env>( .build_bitcast( value_ptr, wrapper_type.ptr_type(AddressSpace::Generic), - "opaque_to_correct", + "opaque_to_correct_recursive_decrement", ) .into_pointer_value(); From 180575852aea9efe412054ef4e54132c6c800d3b Mon Sep 17 00:00:00 2001 From: Folkert Date: Sun, 7 Nov 2021 14:56:24 +0100 Subject: [PATCH 11/44] all tests passing --- compiler/gen_llvm/src/llvm/build.rs | 27 ++++++++++++++++++++--- compiler/gen_llvm/src/llvm/build_hash.rs | 10 ++++++++- compiler/gen_llvm/src/llvm/build_list.rs | 23 ++++++++++++++----- compiler/gen_llvm/src/llvm/refcounting.rs | 19 +++++++++++----- 4 files changed, 65 insertions(+), 14 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 83d6d8d253..35832f8f8d 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -2372,6 +2372,25 @@ fn list_literal<'a, 'ctx, 'env>( } } +pub fn load_roc_value<'a, 'ctx, 'env>( + env: &Env<'a, 'ctx, 'env>, + layout: Layout<'a>, + source: PointerValue<'ctx>, + name: &str, +) -> BasicValueEnum<'ctx> { + if layout.is_passed_by_reference() { + let alloca = env + .builder + .build_alloca(basic_type_from_layout(env, &layout), name); + + store_roc_value(env, layout, alloca, source.into()); + + alloca.into() + } else { + env.builder.build_load(source, name) + } +} + pub fn store_roc_value_opaque<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, layout: Layout<'a>, @@ -3318,7 +3337,7 @@ fn expose_function_to_host_help_c_abi_generic<'a, 'ctx, 'env>( .unwrap() .into_pointer_value(); - builder.build_store(output_arg, call_result); + store_roc_value(env, return_layout, output_arg, call_result); builder.build_return(None); c_function @@ -4218,8 +4237,10 @@ pub fn build_closure_caller<'a, 'ctx, 'env>( // NOTE this may be incorrect in the long run // here we load any argument that is a pointer - for param in evaluator_arguments.iter_mut() { - if param.is_pointer_value() { + let closure_layout = lambda_set.runtime_representation(); + let layouts_it = arguments.iter().chain(std::iter::once(&closure_layout)); + for (param, layout) in evaluator_arguments.iter_mut().zip(layouts_it) { + if param.is_pointer_value() && !layout.is_passed_by_reference() { *param = builder.build_load(param.into_pointer_value(), "load_param"); } } diff --git a/compiler/gen_llvm/src/llvm/build_hash.rs b/compiler/gen_llvm/src/llvm/build_hash.rs index 7064cc100d..b73fe7bd4e 100644 --- a/compiler/gen_llvm/src/llvm/build_hash.rs +++ b/compiler/gen_llvm/src/llvm/build_hash.rs @@ -780,7 +780,15 @@ fn hash_list<'a, 'ctx, 'env>( env.builder.build_store(result, answer); }; - incrementing_elem_loop(env, parent, ptr, length, "current_index", loop_fn); + incrementing_elem_loop( + env, + parent, + *element_layout, + ptr, + length, + "current_index", + loop_fn, + ); env.builder.build_unconditional_branch(done_block); diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index 8b84bb6677..b581c3653e 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -17,6 +17,8 @@ use morphic_lib::UpdateMode; use roc_builtins::bitcode; use roc_mono::layout::{Builtin, Layout, LayoutIds}; +use super::build::load_roc_value; + pub fn pass_update_mode<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, update_mode: UpdateMode, @@ -216,10 +218,15 @@ pub fn list_get_unsafe<'a, 'ctx, 'env>( // Assume the bounds have already been checked earlier // (e.g. by List.get or List.first, which wrap List.#getUnsafe) - let elem_ptr = - unsafe { builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem") }; + let elem_ptr = unsafe { + builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "list_get_element") + }; - let result = builder.build_load(elem_ptr, "List.get"); + let result = if elem_layout.is_passed_by_reference() { + elem_ptr.into() + } else { + builder.build_load(elem_ptr, "list_get_load_element") + }; increment_refcount_layout(env, parent, layout_ids, 1, result, elem_layout); @@ -975,6 +982,7 @@ where pub fn incrementing_elem_loop<'a, 'ctx, 'env, LoopFn>( env: &Env<'a, 'ctx, 'env>, parent: FunctionValue<'ctx>, + element_layout: Layout<'a>, ptr: PointerValue<'ctx>, len: IntValue<'ctx>, index_name: &str, @@ -987,9 +995,14 @@ where incrementing_index_loop(env, parent, len, index_name, |index| { // The pointer to the element in the list - let elem_ptr = unsafe { builder.build_in_bounds_gep(ptr, &[index], "load_index") }; + let element_ptr = unsafe { builder.build_in_bounds_gep(ptr, &[index], "load_index") }; - let elem = builder.build_load(elem_ptr, "get_elem"); + let elem = load_roc_value( + env, + element_layout, + element_ptr, + "incrementing_element_loop_load", + ); loop_fn(index, elem); }) diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index 30c309f540..37f4e4cf42 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -765,7 +765,15 @@ fn modify_refcount_list_help<'a, 'ctx, 'env>( ); }; - incrementing_elem_loop(env, parent, ptr, len, "modify_rc_index", loop_fn); + incrementing_elem_loop( + env, + parent, + *element_layout, + ptr, + len, + "modify_rc_index", + loop_fn, + ); } let refcount_ptr = PointerToRefcount::from_list_wrapper(env, original_wrapper); @@ -1616,7 +1624,6 @@ fn modify_refcount_union<'a, 'ctx, 'env>( None => { let basic_type = basic_type_from_layout_1(env, &layout); let function_value = build_header(env, basic_type, mode, &fn_name); - dbg!(function_value); modify_refcount_union_help( env, @@ -1668,8 +1675,6 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( let before_block = env.builder.get_insert_block().expect("to be in a function"); - dbg!(fn_val, arg_ptr); - // read the tag_id let tag_id_ptr = env .builder @@ -1729,7 +1734,11 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( .build_struct_gep(cast_tag_data_pointer, i as u32, "modify_tag_field") .unwrap(); - let field_value = env.builder.build_load(field_ptr, "field_value"); + let field_value = if field_layout.is_passed_by_reference() { + field_ptr.into() + } else { + env.builder.build_load(field_ptr, "field_value") + }; modify_refcount_layout_help( env, From 3138fc43ec8e9662f381bbc0325c8be52794173e Mon Sep 17 00:00:00 2001 From: Folkert Date: Sun, 7 Nov 2021 16:31:43 +0100 Subject: [PATCH 12/44] cosmetic changes --- compiler/gen_llvm/src/llvm/build.rs | 4 ++-- compiler/gen_llvm/src/llvm/build_list.rs | 6 +----- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index ea4107c91d..f83656da09 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -1536,7 +1536,7 @@ pub fn build_tag<'a, 'ctx, 'env>( // store the tag id let tag_id_ptr = env .builder - .build_struct_gep(result_alloca, TAG_ID_INDEX, "get_opaque_data") + .build_struct_gep(result_alloca, TAG_ID_INDEX, "tag_id_ptr") .unwrap(); let tag_id_intval = tag_id_type.const_int(tag_id as u64, false); @@ -1549,7 +1549,7 @@ pub fn build_tag<'a, 'ctx, 'env>( let struct_opaque_ptr = env .builder - .build_struct_gep(result_alloca, TAG_DATA_INDEX, "get_opaque_data") + .build_struct_gep(result_alloca, TAG_DATA_INDEX, "opaque_data_ptr") .unwrap(); let struct_ptr = env.builder.build_pointer_cast( struct_opaque_ptr, diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index b581c3653e..4c0e7423af 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -222,11 +222,7 @@ pub fn list_get_unsafe<'a, 'ctx, 'env>( builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "list_get_element") }; - let result = if elem_layout.is_passed_by_reference() { - elem_ptr.into() - } else { - builder.build_load(elem_ptr, "list_get_load_element") - }; + let result = load_roc_value(env, **elem_layout, elem_ptr, "list_get_load_element"); increment_refcount_layout(env, parent, layout_ids, 1, result, elem_layout); From e7ec575a81f5a8b335e8e01d3dfc309e4c4e2462 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sun, 7 Nov 2021 21:41:12 +0100 Subject: [PATCH 13/44] trying to track down uninitialized memory creation --- compiler/gen_llvm/src/llvm/build.rs | 79 ++++++++++++++++++----------- compiler/mono/src/alias_analysis.rs | 2 +- 2 files changed, 51 insertions(+), 30 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index f83656da09..480a5ccdfe 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -619,8 +619,8 @@ pub fn construct_optimization_passes<'a>( mpm.add_always_inliner_pass(); // tail-call elimination is always on - // fpm.add_instruction_combining_pass(); - // fpm.add_tail_call_elimination_pass(); + fpm.add_instruction_combining_pass(); + fpm.add_tail_call_elimination_pass(); let pmb = PassManagerBuilder::create(); match opt_level { @@ -1460,6 +1460,36 @@ fn build_wrapped_tag<'a, 'ctx, 'env>( } } +pub fn tag_alloca<'a, 'ctx, 'env>( + env: &Env<'a, 'ctx, 'env>, + type_: BasicTypeEnum<'ctx>, + name: &str, +) -> PointerValue<'ctx> { + let result_alloca = env.builder.build_alloca(type_, name); + + // Initialize all memory of the alloca. This _should_ not be required, but currently + // LLVM can access uninitialized memory after applying some optimizations. Hopefully + // we can in the future adjust code gen so this is no longer an issue. + // + // An example is + // + // main : Task.Task {} [] + // main = + // when List.len [ Ok "foo", Err 42, Ok "spam" ] is + // n -> Task.putLine (Str.fromInt n) + // + // Here the decrement function of result must first check it's an Ok tag, + // then defers to string decrement, which must check is the string is small or large + // + // After inlining, those checks are combined. That means that even if the tag is Err, + // a check is done on the "string" to see if it is big or small, which will touch the + // uninitialized memory. + let all_zeros = type_.const_zero(); + env.builder.build_store(result_alloca, all_zeros); + + result_alloca +} + pub fn build_tag<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, scope: &Scope<'a, 'ctx>, @@ -1482,27 +1512,7 @@ pub fn build_tag<'a, 'ctx, 'env>( let wrapper_type = env .context .struct_type(&[internal_type, tag_id_type.into()], false); - let result_alloca = env.builder.build_alloca(wrapper_type, "tag_opaque"); - - // Initialize all memory of the alloca. This _should_ not be required, but currently - // LLVM can access uninitialized memory after applying some optimizations. Hopefully - // we can in the future adjust code gen so this is no longer an issue. - // - // An example is - // - // main : Task.Task {} [] - // main = - // when List.len [ Ok "foo", Err 42, Ok "spam" ] is - // n -> Task.putLine (Str.fromInt n) - // - // Here the decrement function of result must first check it's an Ok tag, - // then defers to string decrement, which must check is the string is small or large - // - // After inlining, those checks are combined. That means that even if the tag is Err, - // a check is done on the "string" to see if it is big or small, which will touch the - // uninitialized memory. - let all_zeros = wrapper_type.const_zero(); - env.builder.build_store(result_alloca, all_zeros); + let result_alloca = tag_alloca(env, wrapper_type.into(), "opaque_tag"); // Determine types let num_fields = arguments.len() + 1; @@ -2031,7 +2041,7 @@ fn lookup_at_index_ptr2<'a, 'ctx, 'env>( let field_type = basic_type_from_layout(env, &field_layout); let align_bytes = field_layout.alignment_bytes(env.ptr_bytes); - let alloca = env.builder.build_alloca(field_type, "copied_tag"); + let alloca = tag_alloca(env, field_type, "copied_tag"); if align_bytes > 0 { let size = env .ptr_int() @@ -2379,9 +2389,7 @@ pub fn load_roc_value<'a, 'ctx, 'env>( name: &str, ) -> BasicValueEnum<'ctx> { if layout.is_passed_by_reference() { - let alloca = env - .builder - .build_alloca(basic_type_from_layout(env, &layout), name); + let alloca = tag_alloca(env, basic_type_from_layout(env, &layout), name); store_roc_value(env, layout, alloca, source.into()); @@ -2509,7 +2517,20 @@ pub fn build_exp_stmt<'a, 'ctx, 'env>( let out_parameter = parameters.last().unwrap(); debug_assert!(out_parameter.is_pointer_value()); - store_roc_value(env, *layout, out_parameter.into_pointer_value(), value); + // store_roc_value(env, *layout, out_parameter.into_pointer_value(), value); + + let destination = out_parameter.into_pointer_value(); + if layout.is_passed_by_reference() { + let align_bytes = layout.alignment_bytes(env.ptr_bytes); + + if align_bytes > 0 { + let value_ptr = value.into_pointer_value(); + + value_ptr.replace_all_uses_with(destination); + } + } else { + env.builder.build_store(destination, value); + } if let Some(block) = env.builder.get_insert_block() { match block.get_terminator() { @@ -4569,7 +4590,7 @@ pub fn call_roc_function<'a, 'ctx, 'env>( let mut arguments = Vec::from_iter_in(arguments.iter().copied(), env.arena); let result_type = basic_type_from_layout(env, result_layout); - let result_alloca = env.builder.build_alloca(result_type, "result_value"); + let result_alloca = tag_alloca(env, result_type, "result_value"); arguments.push(result_alloca.into()); diff --git a/compiler/mono/src/alias_analysis.rs b/compiler/mono/src/alias_analysis.rs index 2a0d4b1c18..e2f9bd582c 100644 --- a/compiler/mono/src/alias_analysis.rs +++ b/compiler/mono/src/alias_analysis.rs @@ -243,7 +243,7 @@ where match opt_level { OptLevel::Development | OptLevel::Normal => morphic_lib::solve_trivial(program), - OptLevel::Optimize => morphic_lib::solve(program), + OptLevel::Optimize => morphic_lib::solve_trivial(program), } } From 826628456786493018a698b5dda43075fd71ea64 Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 8 Nov 2021 22:31:08 +0100 Subject: [PATCH 14/44] clippy --- compiler/gen_llvm/src/llvm/bitcode.rs | 2 +- compiler/gen_llvm/src/llvm/build.rs | 64 +++++++++++++---------- compiler/gen_llvm/src/llvm/build_hash.rs | 2 +- compiler/gen_llvm/src/llvm/compare.rs | 38 +++++++++++--- compiler/gen_llvm/src/llvm/refcounting.rs | 21 +++----- compiler/load/src/file.rs | 2 +- compiler/mono/src/alias_analysis.rs | 2 +- compiler/test_gen/src/gen_tags.rs | 1 - 8 files changed, 79 insertions(+), 53 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/bitcode.rs b/compiler/gen_llvm/src/llvm/bitcode.rs index fa89ceacf9..0fad482fef 100644 --- a/compiler/gen_llvm/src/llvm/bitcode.rs +++ b/compiler/gen_llvm/src/llvm/bitcode.rs @@ -1,7 +1,7 @@ /// Helpers for interacting with the zig that generates bitcode use crate::debug_info_init; use crate::llvm::build::{struct_from_fields, Env, C_CALL_CONV, FAST_CALL_CONV, TAG_DATA_INDEX}; -use crate::llvm::convert::{basic_type_from_layout, basic_type_from_layout_1}; +use crate::llvm::convert::basic_type_from_layout; use crate::llvm::refcounting::{ decrement_refcount_layout, increment_n_refcount_layout, increment_refcount_layout, }; diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index 480a5ccdfe..66a2e0013b 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -1194,17 +1194,7 @@ pub fn build_exp_expr<'a, 'ctx, 'env>( .unwrap(); let field_layout = fields[*index as usize]; - if field_layout.is_passed_by_reference() { - let alloca = env.builder.build_alloca( - basic_type_from_layout(env, &field_layout), - "struct_field_tag", - ); - env.builder.build_store(alloca, field_value); - - alloca.into() - } else { - field_value - } + use_roc_value(env, field_layout, field_value, "struct_field_tag") } ( PointerValue(argument), @@ -1253,8 +1243,6 @@ pub fn build_exp_expr<'a, 'ctx, 'env>( index, union_layout, } => { - let builder = env.builder; - // cast the argument bytes into the desired shape for this tag let (argument, _structure_layout) = load_symbol_and_layout(scope, structure); @@ -2399,6 +2387,23 @@ pub fn load_roc_value<'a, 'ctx, 'env>( } } +pub fn use_roc_value<'a, 'ctx, 'env>( + env: &Env<'a, 'ctx, 'env>, + layout: Layout<'a>, + source: BasicValueEnum<'ctx>, + name: &str, +) -> BasicValueEnum<'ctx> { + if layout.is_passed_by_reference() { + let alloca = tag_alloca(env, basic_type_from_layout(env, &layout), name); + + env.builder.build_store(alloca, source); + + alloca.into() + } else { + source + } +} + pub fn store_roc_value_opaque<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, layout: Layout<'a>, @@ -2526,7 +2531,23 @@ pub fn build_exp_stmt<'a, 'ctx, 'env>( if align_bytes > 0 { let value_ptr = value.into_pointer_value(); - value_ptr.replace_all_uses_with(destination); + if true { + value_ptr.replace_all_uses_with(destination); + } else { + let size = env + .ptr_int() + .const_int(layout.stack_size(env.ptr_bytes) as u64, false); + + env.builder + .build_memcpy( + destination, + align_bytes, + value.into_pointer_value(), + align_bytes, + size, + ) + .unwrap(); + } } } else { env.builder.build_store(destination, value); @@ -2794,20 +2815,6 @@ pub fn load_symbol_and_lambda_set<'a, 'ctx, 'b>( } } -fn access_index_struct_value<'ctx>( - builder: &Builder<'ctx>, - from_value: StructValue<'ctx>, - to_type: StructType<'ctx>, -) -> StructValue<'ctx> { - complex_bitcast( - builder, - from_value.into(), - to_type.into(), - "access_index_struct_value", - ) - .into_struct_value() -} - /// Cast a value to another value of the same (or smaller?) size pub fn cast_basic_basic<'ctx>( builder: &Builder<'ctx>, @@ -4660,6 +4667,7 @@ pub struct RocFunctionCall<'ctx> { pub data_is_owned: IntValue<'ctx>, } +#[allow(clippy::too_many_arguments)] fn roc_function_call<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, layout_ids: &mut LayoutIds<'a>, diff --git a/compiler/gen_llvm/src/llvm/build_hash.rs b/compiler/gen_llvm/src/llvm/build_hash.rs index b73fe7bd4e..261af4e7d4 100644 --- a/compiler/gen_llvm/src/llvm/build_hash.rs +++ b/compiler/gen_llvm/src/llvm/build_hash.rs @@ -2,7 +2,7 @@ use crate::debug_info_init; use crate::llvm::bitcode::call_bitcode_fn; use crate::llvm::build::tag_pointer_clear_tag_id; use crate::llvm::build::Env; -use crate::llvm::build::{cast_block_of_memory_to_tag, get_tag_id, FAST_CALL_CONV, TAG_DATA_INDEX}; +use crate::llvm::build::{get_tag_id, FAST_CALL_CONV, TAG_DATA_INDEX}; use crate::llvm::build_str; use crate::llvm::convert::{basic_type_from_layout, basic_type_from_layout_1}; use bumpalo::collections::Vec; diff --git a/compiler/gen_llvm/src/llvm/compare.rs b/compiler/gen_llvm/src/llvm/compare.rs index 83c8887ce8..c624890616 100644 --- a/compiler/gen_llvm/src/llvm/compare.rs +++ b/compiler/gen_llvm/src/llvm/compare.rs @@ -884,8 +884,15 @@ fn build_tag_eq_help<'a, 'ctx, 'env>( let block = env.context.append_basic_block(parent, "tag_id_modify"); env.builder.position_at_end(block); - let answer = - eq_ptr_to_struct(env, layout_ids, union_layout, field_layouts, tag1, tag2); + let answer = eq_ptr_to_struct( + env, + layout_ids, + union_layout, + Some(when_recursive.clone()), + field_layouts, + tag1, + tag2, + ); env.builder.build_return(Some(&answer)); @@ -941,8 +948,15 @@ fn build_tag_eq_help<'a, 'ctx, 'env>( let block = env.context.append_basic_block(parent, "tag_id_modify"); env.builder.position_at_end(block); - let answer = - eq_ptr_to_struct(env, layout_ids, union_layout, field_layouts, tag1, tag2); + let answer = eq_ptr_to_struct( + env, + layout_ids, + union_layout, + None, + field_layouts, + tag1, + tag2, + ); env.builder.build_return(Some(&answer)); @@ -998,6 +1012,7 @@ fn build_tag_eq_help<'a, 'ctx, 'env>( env, layout_ids, union_layout, + None, other_fields, tag1.into_pointer_value(), tag2.into_pointer_value(), @@ -1088,8 +1103,15 @@ fn build_tag_eq_help<'a, 'ctx, 'env>( let block = env.context.append_basic_block(parent, "tag_id_modify"); env.builder.position_at_end(block); - let answer = - eq_ptr_to_struct(env, layout_ids, union_layout, field_layouts, tag1, tag2); + let answer = eq_ptr_to_struct( + env, + layout_ids, + union_layout, + None, + field_layouts, + tag1, + tag2, + ); env.builder.build_return(Some(&answer)); @@ -1123,6 +1145,7 @@ fn build_tag_eq_help<'a, 'ctx, 'env>( env, layout_ids, union_layout, + None, field_layouts, tag1.into_pointer_value(), tag2.into_pointer_value(), @@ -1137,6 +1160,7 @@ fn eq_ptr_to_struct<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, layout_ids: &mut LayoutIds<'a>, union_layout: &UnionLayout<'a>, + opt_when_recursive: Option>, field_layouts: &'a [Layout<'a>], tag1: PointerValue<'ctx>, tag2: PointerValue<'ctx>, @@ -1179,7 +1203,7 @@ fn eq_ptr_to_struct<'a, 'ctx, 'env>( env, layout_ids, field_layouts, - WhenRecursive::Loop(*union_layout), + opt_when_recursive.unwrap_or(WhenRecursive::Loop(*union_layout)), struct1, struct2, ) diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index 9c40a8d367..de55b101ff 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -1,8 +1,8 @@ use crate::debug_info_init; use crate::llvm::bitcode::call_void_bitcode_fn; use crate::llvm::build::{ - add_func, cast_basic_basic, get_tag_id, tag_pointer_clear_tag_id, Env, FAST_CALL_CONV, - TAG_DATA_INDEX, TAG_ID_INDEX, + add_func, cast_basic_basic, get_tag_id, tag_pointer_clear_tag_id, use_roc_value, Env, + FAST_CALL_CONV, TAG_DATA_INDEX, TAG_ID_INDEX, }; use crate::llvm::build_list::{incrementing_elem_loop, list_len, load_list}; use crate::llvm::convert::{basic_type_from_layout, basic_type_from_layout_1, ptr_int}; @@ -354,17 +354,12 @@ fn modify_refcount_struct_help<'a, 'ctx, 'env>( .build_extract_value(wrapper_struct, i as u32, "decrement_struct_field") .unwrap(); - let field_value = if field_layout.is_passed_by_reference() { - let field_type = basic_type_from_layout(env, field_layout); - let alloca = env - .builder - .build_alloca(field_type, "load_struct_tag_field_for_decrement"); - env.builder.build_store(alloca, raw_value); - - alloca.into() - } else { - raw_value - }; + let field_value = use_roc_value( + env, + *field_layout, + raw_value, + "load_struct_tag_field_for_decrement", + ); modify_refcount_layout_help( env, diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index 60e8d7a5f2..e0dfe8fd0a 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -2152,7 +2152,7 @@ fn update<'a>( println!("{}", result); } - Proc::insert_refcount_operations(arena, &mut state.procedures); + // Proc::insert_refcount_operations(arena, &mut state.procedures); // This is not safe with the new non-recursive RC updates that we do for tag unions // diff --git a/compiler/mono/src/alias_analysis.rs b/compiler/mono/src/alias_analysis.rs index e2f9bd582c..2a0d4b1c18 100644 --- a/compiler/mono/src/alias_analysis.rs +++ b/compiler/mono/src/alias_analysis.rs @@ -243,7 +243,7 @@ where match opt_level { OptLevel::Development | OptLevel::Normal => morphic_lib::solve_trivial(program), - OptLevel::Optimize => morphic_lib::solve_trivial(program), + OptLevel::Optimize => morphic_lib::solve(program), } } diff --git a/compiler/test_gen/src/gen_tags.rs b/compiler/test_gen/src/gen_tags.rs index 9c5989b174..c62140fd96 100644 --- a/compiler/test_gen/src/gen_tags.rs +++ b/compiler/test_gen/src/gen_tags.rs @@ -3,7 +3,6 @@ use crate::assert_evals_to; // use crate::assert_wasm_evals_to as assert_evals_to; use indoc::indoc; -use roc_mono::layout::LambdaSet; use roc_std::{RocList, RocStr}; #[test] From 7ff4ad6f7ba6aa75dab5df3702d05ba572de6a1a Mon Sep 17 00:00:00 2001 From: Folkert Date: Mon, 8 Nov 2021 23:57:56 +0100 Subject: [PATCH 15/44] fix merge conflict --- compiler/gen_llvm/src/llvm/build.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index ba9b67a858..642332171b 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -5166,6 +5166,7 @@ fn run_higher_order_low_level<'a, 'ctx, 'env>( closure_layout, function_owns_closure_data, argument_layouts, + Layout::Builtin(Builtin::Int1), ); list_any(env, roc_function_call, list, element_layout) From 4fdb8d354b501120972d736d026693b966eef20b Mon Sep 17 00:00:00 2001 From: Folkert Date: Wed, 10 Nov 2021 00:22:49 +0100 Subject: [PATCH 16/44] turn on refcounting again, turning it off does not help (builtins still decrement and potentially free) --- compiler/load/src/file.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index e0dfe8fd0a..60e8d7a5f2 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -2152,7 +2152,7 @@ fn update<'a>( println!("{}", result); } - // Proc::insert_refcount_operations(arena, &mut state.procedures); + Proc::insert_refcount_operations(arena, &mut state.procedures); // This is not safe with the new non-recursive RC updates that we do for tag unions // From 38da99b1ac017abda1b8eb9274cce54e131410bd Mon Sep 17 00:00:00 2001 From: Folkert Date: Thu, 11 Nov 2021 23:36:35 +0100 Subject: [PATCH 17/44] make it work --- cli/src/repl/gen.rs | 4 ++-- compiler/gen_llvm/src/llvm/bitcode.rs | 15 ++++----------- compiler/gen_llvm/src/llvm/build.rs | 13 +++++++------ compiler/gen_llvm/src/llvm/refcounting.rs | 23 +++++++++++++++++++---- 4 files changed, 32 insertions(+), 23 deletions(-) diff --git a/cli/src/repl/gen.rs b/cli/src/repl/gen.rs index 200941919d..3e789da85b 100644 --- a/cli/src/repl/gen.rs +++ b/cli/src/repl/gen.rs @@ -218,8 +218,8 @@ pub fn gen_and_eval<'a>( // Verify the module if let Err(errors) = env.module.verify() { panic!( - "Errors defining module: {}\n\nUncomment things nearby to see more details.", - errors + "Errors defining module:\n{}\n\nUncomment things nearby to see more details.", + errors.to_string() ); } diff --git a/compiler/gen_llvm/src/llvm/bitcode.rs b/compiler/gen_llvm/src/llvm/bitcode.rs index 0fad482fef..2afb1c7159 100644 --- a/compiler/gen_llvm/src/llvm/bitcode.rs +++ b/compiler/gen_llvm/src/llvm/bitcode.rs @@ -154,18 +154,11 @@ fn build_has_tag_id_help<'a, 'ctx, 'env>( ); let tag_data_ptr = { - let data_index = env - .context - .i64_type() - .const_int(TAG_DATA_INDEX as u64, false); + let ptr = env + .builder + .build_struct_gep(tag_value, TAG_DATA_INDEX, "get_data_ptr") + .unwrap(); - let ptr = unsafe { - env.builder.build_gep( - tag_value_ptr.into_pointer_value(), - &[data_index], - "get_data_ptr", - ) - }; env.builder.build_bitcast(ptr, i8_ptr_type, "to_opaque") }; diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index b89096f4a5..711116bc80 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -2532,7 +2532,10 @@ pub fn build_exp_stmt<'a, 'ctx, 'env>( if align_bytes > 0 { let value_ptr = value.into_pointer_value(); - if true { + // We can only do this if the function itself writes data into this + // pointer. If the pointer is passed as an argument, then we must copy + // from one pointer to our destination pointer + if value_ptr.get_first_use().is_some() { value_ptr.replace_all_uses_with(destination); } else { let size = env @@ -3382,8 +3385,7 @@ fn expose_function_to_host_help_c_abi_gen_test<'a, 'ctx, 'env>( // a tagged union to indicate to the test loader that a panic occurred. // especially when running 32-bit binaries on a 64-bit machine, there // does not seem to be a smarter solution - let wrapper_return_type = - roc_result_type(env, roc_function.get_type().get_return_type().unwrap()); + let wrapper_return_type = roc_result_type(env, basic_type_from_layout(env, &return_layout)); let mut cc_argument_types = Vec::with_capacity_in(arguments.len(), env.arena); for layout in arguments { @@ -3861,7 +3863,7 @@ fn make_good_roc_result<'a, 'ctx, 'env>( let context = env.context; let builder = env.builder; - let v1 = roc_result_type(env, return_value.get_type()).const_zero(); + let v1 = roc_result_type(env, basic_type_from_layout(env, &return_layout)).const_zero(); let v2 = builder .build_insert_value(v1, context.i64_type().const_zero(), 0, "set_no_error") @@ -3906,8 +3908,7 @@ fn make_exception_catching_wrapper<'a, 'ctx, 'env>( } }; - let wrapper_return_type = - roc_result_type(env, roc_function.get_type().get_return_type().unwrap()); + let wrapper_return_type = roc_result_type(env, basic_type_from_layout(env, &return_layout)); // argument_types.push(wrapper_return_type.ptr_type(AddressSpace::Generic).into()); diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index de55b101ff..0a956f8bce 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -139,8 +139,10 @@ impl<'ctx> PointerToRefcount<'ctx> { let block = env.builder.get_insert_block().expect("to be in a function"); let parent = block.get_parent().unwrap(); - let modify_block = env.context.append_basic_block(parent, "inc_str_modify"); - let cont_block = env.context.append_basic_block(parent, "inc_str_cont"); + let modify_block = env + .context + .append_basic_block(parent, "inc_refcount_modify"); + let cont_block = env.context.append_basic_block(parent, "inc_refcount_cont"); env.builder .build_conditional_branch(is_static_allocation, cont_block, modify_block); @@ -1718,12 +1720,25 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( "cast_to_concrete_tag", ); - // let wrapper_struct = cast_block_of_memory_to_tag(env.builder, data_bytes, wrapper_type); - for (i, field_layout) in field_layouts.iter().enumerate() { if let Layout::RecursivePointer = field_layout { panic!("non-recursive tag unions cannot contain naked recursion pointers!"); } else if field_layout.contains_refcounted() { + // crazy hack + match field_layout { + Layout::Builtin(Builtin::Str | Builtin::List(_)) => { + use inkwell::attributes::{Attribute, AttributeLoc}; + let kind_id = Attribute::get_named_enum_kind_id("noinline"); + debug_assert!(kind_id > 0); + let enum_attr = env.context.create_enum_attribute(kind_id, 1); + + fn_val.add_attribute(AttributeLoc::Function, enum_attr); + } + _ => { + // do nothing + } + } + let field_ptr = env .builder .build_struct_gep(cast_tag_data_pointer, i as u32, "modify_tag_field") From 65a9febe7dd1e6294d34619d5dd00e151fafea68 Mon Sep 17 00:00:00 2001 From: Folkert Date: Thu, 11 Nov 2021 23:38:45 +0100 Subject: [PATCH 18/44] clippy --- compiler/gen_llvm/src/llvm/refcounting.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index 0a956f8bce..47f2ecf24c 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -1724,19 +1724,15 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( if let Layout::RecursivePointer = field_layout { panic!("non-recursive tag unions cannot contain naked recursion pointers!"); } else if field_layout.contains_refcounted() { - // crazy hack - match field_layout { - Layout::Builtin(Builtin::Str | Builtin::List(_)) => { - use inkwell::attributes::{Attribute, AttributeLoc}; - let kind_id = Attribute::get_named_enum_kind_id("noinline"); - debug_assert!(kind_id > 0); - let enum_attr = env.context.create_enum_attribute(kind_id, 1); + // crazy hack: inlining this function when it decrements a list or string results + // in many, many valgrind errors in `--optimize` mode. We do not know why. + if let Layout::Builtin(Builtin::Str | Builtin::List(_)) = field_layout { + use inkwell::attributes::{Attribute, AttributeLoc}; + let kind_id = Attribute::get_named_enum_kind_id("noinline"); + debug_assert!(kind_id > 0); + let enum_attr = env.context.create_enum_attribute(kind_id, 1); - fn_val.add_attribute(AttributeLoc::Function, enum_attr); - } - _ => { - // do nothing - } + fn_val.add_attribute(AttributeLoc::Function, enum_attr); } let field_ptr = env From d5301817b73a8fc1792a1e864bbe529066786dbc Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Thu, 11 Nov 2021 20:04:37 -0800 Subject: [PATCH 19/44] Apply `cargo fmt --all` to Cargo.toml --- cli_utils/Cargo.lock | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/cli_utils/Cargo.lock b/cli_utils/Cargo.lock index f41f503d27..4889a1da49 100644 --- a/cli_utils/Cargo.lock +++ b/cli_utils/Cargo.lock @@ -2398,8 +2398,6 @@ name = "roc_build" version = "0.1.0" dependencies = [ "bumpalo", - "im", - "im-rc", "inkwell 0.1.0", "libloading 0.7.1", "roc_builtins", @@ -2440,8 +2438,6 @@ name = "roc_can" version = "0.1.0" dependencies = [ "bumpalo", - "im", - "im-rc", "roc_builtins", "roc_collections", "roc_module", @@ -2459,8 +2455,6 @@ dependencies = [ "bumpalo", "clap 3.0.0-beta.5", "const_format", - "im", - "im-rc", "inkwell 0.1.0", "libloading 0.7.1", "mimalloc", @@ -2562,8 +2556,6 @@ dependencies = [ "fs_extra", "futures", "glyph_brush", - "im", - "im-rc", "libc", "log", "nonempty", @@ -2599,8 +2591,6 @@ name = "roc_fmt" version = "0.1.0" dependencies = [ "bumpalo", - "im", - "im-rc", "roc_collections", "roc_module", "roc_parse", @@ -2612,8 +2602,6 @@ name = "roc_gen_dev" version = "0.1.0" dependencies = [ "bumpalo", - "im", - "im-rc", "object 0.26.2", "roc_builtins", "roc_collections", @@ -2632,20 +2620,13 @@ name = "roc_gen_llvm" version = "0.1.0" dependencies = [ "bumpalo", - "im", - "im-rc", "inkwell 0.1.0", "morphic_lib", "roc_builtins", "roc_collections", "roc_module", "roc_mono", - "roc_problem", - "roc_region", - "roc_solve", "roc_std", - "roc_types", - "roc_unify", "target-lexicon", ] @@ -2654,6 +2635,7 @@ name = "roc_gen_wasm" version = "0.1.0" dependencies = [ "bumpalo", + "roc_builtins", "roc_collections", "roc_module", "roc_mono", @@ -2726,7 +2708,6 @@ version = "0.1.0" dependencies = [ "bumpalo", "hashbrown 0.11.2", - "linked-hash-map", "morphic_lib", "roc_can", "roc_collections", @@ -2737,7 +2718,6 @@ dependencies = [ "roc_std", "roc_types", "roc_unify", - "ven_ena", "ven_graph", "ven_pretty", ] @@ -2773,8 +2753,6 @@ version = "0.1.0" dependencies = [ "bumpalo", "distance", - "im", - "im-rc", "roc_can", "roc_collections", "roc_module", From 196538cc586be523ad8dd795c04d8fd943a34787 Mon Sep 17 00:00:00 2001 From: Folkert Date: Sat, 13 Nov 2021 01:00:20 +0100 Subject: [PATCH 20/44] fix valgrind error, finally --- compiler/gen_llvm/src/llvm/bitcode.rs | 4 ++-- compiler/gen_llvm/src/llvm/build_list.rs | 24 +++++++++++++---------- compiler/gen_llvm/src/llvm/refcounting.rs | 11 ----------- 3 files changed, 16 insertions(+), 23 deletions(-) diff --git a/compiler/gen_llvm/src/llvm/bitcode.rs b/compiler/gen_llvm/src/llvm/bitcode.rs index 2afb1c7159..159c688580 100644 --- a/compiler/gen_llvm/src/llvm/bitcode.rs +++ b/compiler/gen_llvm/src/llvm/bitcode.rs @@ -260,7 +260,7 @@ fn build_transform_caller_help<'a, 'ctx, 'env>( .build_pointer_cast( argument_ptr.into_pointer_value(), basic_type, - "cast_ptr_to_tag", + "cast_ptr_to_tag_build_transform_caller_help", ) .into() } else { @@ -409,7 +409,7 @@ fn build_rc_wrapper<'a, 'ctx, 'env>( let value = if layout.is_passed_by_reference() { env.builder - .build_pointer_cast(value_ptr, value_type, "cast_ptr_to_tag") + .build_pointer_cast(value_ptr, value_type, "cast_ptr_to_tag_build_rc_wrapper") .into() } else { let value_cast = env diff --git a/compiler/gen_llvm/src/llvm/build_list.rs b/compiler/gen_llvm/src/llvm/build_list.rs index 154295d9cd..323960353d 100644 --- a/compiler/gen_llvm/src/llvm/build_list.rs +++ b/compiler/gen_llvm/src/llvm/build_list.rs @@ -17,7 +17,7 @@ use morphic_lib::UpdateMode; use roc_builtins::bitcode; use roc_mono::layout::{Builtin, Layout, LayoutIds}; -use super::build::load_roc_value; +use super::build::{load_roc_value, store_roc_value}; pub fn pass_update_mode<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, @@ -55,9 +55,13 @@ pub fn call_bitcode_fn_returns_list<'a, 'ctx, 'env>( fn pass_element_as_opaque<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, element: BasicValueEnum<'ctx>, + layout: Layout<'a>, ) -> BasicValueEnum<'ctx> { - let element_ptr = env.builder.build_alloca(element.get_type(), "element"); - env.builder.build_store(element_ptr, element); + let element_type = basic_type_from_layout(env, &layout); + let element_ptr = env + .builder + .build_alloca(element_type, "element_to_pass_as_opaque"); + store_roc_value(env, layout, element_ptr, element); env.builder.build_bitcast( element_ptr, @@ -108,7 +112,7 @@ pub fn list_single<'a, 'ctx, 'env>( env, &[ env.alignment_intvalue(element_layout), - pass_element_as_opaque(env, element), + pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), ], bitcode::LIST_SINGLE, @@ -130,7 +134,7 @@ pub fn list_repeat<'a, 'ctx, 'env>( &[ list_len.into(), env.alignment_intvalue(element_layout), - pass_element_as_opaque(env, element), + pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), inc_element_fn.as_global_value().as_pointer_value().into(), ], @@ -250,7 +254,7 @@ pub fn list_append<'a, 'ctx, 'env>( &[ pass_list_cc(env, original_wrapper.into()), env.alignment_intvalue(element_layout), - pass_element_as_opaque(env, element), + pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), pass_update_mode(env, update_mode), ], @@ -270,7 +274,7 @@ pub fn list_prepend<'a, 'ctx, 'env>( &[ pass_list_cc(env, original_wrapper.into()), env.alignment_intvalue(element_layout), - pass_element_as_opaque(env, element), + pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), ], bitcode::LIST_PREPEND, @@ -409,7 +413,7 @@ pub fn list_set<'a, 'ctx, 'env>( &[ bytes.into(), index.into(), - pass_element_as_opaque(env, element), + pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), dec_element_fn.as_global_value().as_pointer_value().into(), ], @@ -422,7 +426,7 @@ pub fn list_set<'a, 'ctx, 'env>( length.into(), env.alignment_intvalue(element_layout), index.into(), - pass_element_as_opaque(env, element), + pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), dec_element_fn.as_global_value().as_pointer_value().into(), ], @@ -598,7 +602,7 @@ pub fn list_contains<'a, 'ctx, 'env>( env, &[ pass_list_cc(env, list), - pass_element_as_opaque(env, element), + pass_element_as_opaque(env, element, *element_layout), layout_width(env, element_layout), eq_fn, ], diff --git a/compiler/gen_llvm/src/llvm/refcounting.rs b/compiler/gen_llvm/src/llvm/refcounting.rs index 47f2ecf24c..1b6c6aa4d0 100644 --- a/compiler/gen_llvm/src/llvm/refcounting.rs +++ b/compiler/gen_llvm/src/llvm/refcounting.rs @@ -1724,17 +1724,6 @@ fn modify_refcount_union_help<'a, 'ctx, 'env>( if let Layout::RecursivePointer = field_layout { panic!("non-recursive tag unions cannot contain naked recursion pointers!"); } else if field_layout.contains_refcounted() { - // crazy hack: inlining this function when it decrements a list or string results - // in many, many valgrind errors in `--optimize` mode. We do not know why. - if let Layout::Builtin(Builtin::Str | Builtin::List(_)) = field_layout { - use inkwell::attributes::{Attribute, AttributeLoc}; - let kind_id = Attribute::get_named_enum_kind_id("noinline"); - debug_assert!(kind_id > 0); - let enum_attr = env.context.create_enum_attribute(kind_id, 1); - - fn_val.add_attribute(AttributeLoc::Function, enum_attr); - } - let field_ptr = env .builder .build_struct_gep(cast_tag_data_pointer, i as u32, "modify_tag_field") From a4ca6a31a643bacea790daa47a348625688887c8 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 08:17:59 -0800 Subject: [PATCH 21/44] Use Collection in Expr::Record and related places --- ast/src/lang/core/expr/expr_to_expr2.rs | 10 +++---- cli/src/repl/eval.rs | 27 +++++-------------- compiler/can/src/expr.rs | 10 +++---- compiler/can/src/operator.rs | 27 ++++++++----------- compiler/can/src/pattern.rs | 2 +- compiler/fmt/src/expr.rs | 26 ++++++++---------- compiler/parse/src/ast.rs | 24 +++++++++++------ compiler/parse/src/expr.rs | 26 ++++++++++-------- compiler/parse/src/pattern.rs | 5 +--- compiler/parse/tests/test_parse.rs | 36 +++++++++++++------------ 10 files changed, 86 insertions(+), 107 deletions(-) diff --git a/ast/src/lang/core/expr/expr_to_expr2.rs b/ast/src/lang/core/expr/expr_to_expr2.rs index d9eaf2cf18..00fa759f65 100644 --- a/ast/src/lang/core/expr/expr_to_expr2.rs +++ b/ast/src/lang/core/expr/expr_to_expr2.rs @@ -185,13 +185,12 @@ pub fn expr_to_expr2<'a>( RecordUpdate { fields, update: loc_update, - final_comments: _, } => { let (can_update, update_out) = expr_to_expr2(env, scope, &loc_update.value, loc_update.region); if let Expr2::Var(symbol) = &can_update { - match canonicalize_fields(env, scope, fields) { + match canonicalize_fields(env, scope, fields.items) { Ok((can_fields, mut output)) => { output.references.union_mut(update_out.references); @@ -236,14 +235,11 @@ pub fn expr_to_expr2<'a>( } } - Record { - fields, - final_comments: _, - } => { + Record(fields) => { if fields.is_empty() { (Expr2::EmptyRecord, Output::default()) } else { - match canonicalize_fields(env, scope, fields) { + match canonicalize_fields(env, scope, fields.items) { Ok((can_fields, output)) => ( Expr2::Record { record_var: env.var_store.fresh(), diff --git a/cli/src/repl/eval.rs b/cli/src/repl/eval.rs index 307b097419..b56200dc6f 100644 --- a/cli/src/repl/eval.rs +++ b/cli/src/repl/eval.rs @@ -7,7 +7,7 @@ use roc_module::operator::CalledVia; use roc_module::symbol::{Interns, ModuleId, Symbol}; use roc_mono::ir::ProcLayout; use roc_mono::layout::{union_sorted_tags_help, Builtin, Layout, UnionLayout, UnionVariant}; -use roc_parse::ast::{AssignedField, Expr, StrLiteral}; +use roc_parse::ast::{AssignedField, Collection, Expr, StrLiteral}; use roc_region::all::{Located, Region}; use roc_types::subs::{Content, FlatType, GetSubsSlice, RecordFields, Subs, UnionTags, Variable}; @@ -621,10 +621,7 @@ fn struct_to_ast<'a>( let output = env.arena.alloc([loc_field]); - Expr::Record { - fields: output, - final_comments: &[], - } + Expr::Record(Collection::with_items(output)) } else { debug_assert_eq!(sorted_fields.len(), field_layouts.len()); @@ -658,10 +655,7 @@ fn struct_to_ast<'a>( let output = output.into_bump_slice(); - Expr::Record { - fields: output, - final_comments: &[], - } + Expr::Record(Collection::with_items(output)) } } @@ -735,10 +729,7 @@ fn bool_to_ast<'a>(env: &Env<'a, '_>, value: bool, content: &Content) -> Expr<'a region: Region::zero(), }; - Expr::Record { - fields: arena.alloc([loc_assigned_field]), - final_comments: arena.alloc([]), - } + Expr::Record(Collection::with_items(arena.alloc([loc_assigned_field]))) } FlatType::TagUnion(tags, _) if tags.len() == 1 => { let (tag_name, payload_vars) = unpack_single_element_tag_union(env.subs, *tags); @@ -850,10 +841,7 @@ fn byte_to_ast<'a>(env: &Env<'a, '_>, value: u8, content: &Content) -> Expr<'a> region: Region::zero(), }; - Expr::Record { - fields: arena.alloc([loc_assigned_field]), - final_comments: &[], - } + Expr::Record(Collection::with_items(arena.alloc([loc_assigned_field]))) } FlatType::TagUnion(tags, _) if tags.len() == 1 => { let (tag_name, payload_vars) = unpack_single_element_tag_union(env.subs, *tags); @@ -972,10 +960,7 @@ fn num_to_ast<'a>(env: &Env<'a, '_>, num_expr: Expr<'a>, content: &Content) -> E region: Region::zero(), }; - Expr::Record { - fields: arena.alloc([loc_assigned_field]), - final_comments: arena.alloc([]), - } + Expr::Record(Collection::with_items(arena.alloc([loc_assigned_field]))) } FlatType::TagUnion(tags, _) => { // This was a single-tag union that got unwrapped at runtime. diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 4e96b58e51..768bba827e 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -228,14 +228,11 @@ pub fn canonicalize_expr<'a>( (answer, Output::default()) } - ast::Expr::Record { - fields, - final_comments: _, - } => { + ast::Expr::Record(fields) => { if fields.is_empty() { (EmptyRecord, Output::default()) } else { - match canonicalize_fields(env, var_store, scope, region, fields) { + match canonicalize_fields(env, var_store, scope, region, fields.items) { Ok((can_fields, output)) => ( Record { record_var: var_store.fresh(), @@ -261,12 +258,11 @@ pub fn canonicalize_expr<'a>( ast::Expr::RecordUpdate { fields, update: loc_update, - final_comments: _, } => { let (can_update, update_out) = canonicalize_expr(env, var_store, scope, loc_update.region, &loc_update.value); if let Var(symbol) = &can_update.value { - match canonicalize_fields(env, var_store, scope, region, fields) { + match canonicalize_fields(env, var_store, scope, region, fields.items) { Ok((can_fields, mut output)) => { output.references = output.references.union(update_out.references); diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 36b790764d..4faecdf9bf 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -6,7 +6,7 @@ use roc_module::ident::ModuleName; use roc_module::operator::BinOp::Pizza; use roc_module::operator::{BinOp, CalledVia}; use roc_parse::ast::Expr::{self, *}; -use roc_parse::ast::{AssignedField, Def, WhenBranch}; +use roc_parse::ast::{AssignedField, Collection, Def, WhenBranch}; use roc_region::all::{Located, Region}; // BinOp precedence logic adapted from Gluon by Markus Westerlind @@ -164,10 +164,7 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a value, }) } - Record { - fields, - final_comments, - } => { + Record(fields) => { let mut new_fields = Vec::with_capacity_in(fields.len(), arena); for field in fields.iter() { @@ -183,18 +180,14 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a arena.alloc(Located { region: loc_expr.region, - value: Record { - fields: new_fields, - final_comments, - }, + value: Record(Collection { + items: new_fields, + final_comments: fields.final_comments, + }), }) } - RecordUpdate { - fields, - update, - final_comments, - } => { + RecordUpdate { fields, update } => { // NOTE the `update` field is always a `Var { .. }` and does not need to be desugared let mut new_fields = Vec::with_capacity_in(fields.len(), arena); @@ -213,8 +206,10 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a region: loc_expr.region, value: RecordUpdate { update: *update, - fields: new_fields, - final_comments, + fields: Collection { + items: new_fields, + final_comments: fields.final_comments, + }, }, }) } diff --git a/compiler/can/src/pattern.rs b/compiler/can/src/pattern.rs index adc853bd69..01f0c55862 100644 --- a/compiler/can/src/pattern.rs +++ b/compiler/can/src/pattern.rs @@ -246,7 +246,7 @@ pub fn canonicalize_pattern<'a>( let mut destructs = Vec::with_capacity(patterns.len()); let mut opt_erroneous = None; - for loc_pattern in *patterns { + for loc_pattern in patterns.iter() { match loc_pattern.value { Identifier(label) => { match scope.introduce( diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index 49a0f5f937..fd4d43e821 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -5,7 +5,9 @@ use crate::spaces::{add_spaces, fmt_comments_only, fmt_spaces, newline, NewlineA use bumpalo::collections::String; use roc_module::operator::{self, BinOp}; use roc_parse::ast::StrSegment; -use roc_parse::ast::{AssignedField, Base, CommentOrNewline, Expr, Pattern, WhenBranch}; +use roc_parse::ast::{ + AssignedField, Base, Collection, CommentOrNewline, Expr, Pattern, WhenBranch, +}; use roc_region::all::Located; impl<'a> Formattable<'a> for Expr<'a> { @@ -98,7 +100,7 @@ impl<'a> Formattable<'a> for Expr<'a> { .any(|loc_pattern| loc_pattern.is_multiline()) } - Record { fields, .. } => fields.iter().any(|loc_field| loc_field.is_multiline()), + Record(fields) => fields.iter().any(|loc_field| loc_field.is_multiline()), RecordUpdate { fields, .. } => fields.iter().any(|loc_field| loc_field.is_multiline()), } } @@ -250,18 +252,11 @@ impl<'a> Formattable<'a> for Expr<'a> { buf.push_str(string); } - Record { - fields, - final_comments, - } => { - fmt_record(buf, None, fields, final_comments, indent); + Record(fields) => { + fmt_record(buf, None, *fields, indent); } - RecordUpdate { - fields, - update, - final_comments, - } => { - fmt_record(buf, Some(*update), fields, final_comments, indent); + RecordUpdate { update, fields } => { + fmt_record(buf, Some(*update), *fields, indent); } Closure(loc_patterns, loc_ret) => { fmt_closure(buf, loc_patterns, loc_ret, indent); @@ -917,10 +912,11 @@ fn fmt_backpassing<'a>( fn fmt_record<'a>( buf: &mut String<'a>, update: Option<&'a Located>>, - loc_fields: &[Located>>], - final_comments: &'a [CommentOrNewline<'a>], + fields: Collection<'a, Located>>>, indent: u16, ) { + let loc_fields = fields.items; + let final_comments = fields.final_comments; if loc_fields.is_empty() && final_comments.iter().all(|c| c.is_newline()) { buf.push_str("{}"); } else { diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index 6d31884d56..02fd3acb03 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -25,6 +25,18 @@ impl<'a, T> Collection<'a, T> { final_comments: &[], } } + + pub fn iter(&self) -> impl Iterator { + self.items.iter() + } + + pub fn len(&self) -> usize { + self.items.len() + } + + pub fn is_empty(&self) -> bool { + self.items.is_empty() + } } #[derive(Clone, Debug, PartialEq)] @@ -122,14 +134,10 @@ pub enum Expr<'a> { RecordUpdate { update: &'a Loc>, - fields: &'a [Loc>>], - final_comments: &'a &'a [CommentOrNewline<'a>], + fields: Collection<'a, Loc>>>, }, - Record { - fields: &'a [Loc>>], - final_comments: &'a [CommentOrNewline<'a>], - }, + Record(Collection<'a, Loc>>>), // Lookups Var { @@ -257,7 +265,6 @@ pub enum TypeAnnotation<'a> { /// The row type variable in an open record, e.g. the `r` in `{ name: Str }r`. /// This is None if it's a closed record annotation like `{ name: Str }`. ext: Option<&'a Loc>>, - // final_comments: &'a [CommentOrNewline<'a>], }, /// A tag union, e.g. `[ @@ -357,10 +364,11 @@ pub enum Pattern<'a> { GlobalTag(&'a str), PrivateTag(&'a str), Apply(&'a Loc>, &'a [Loc>]), + /// This is Loc rather than Loc so we can record comments /// around the destructured names, e.g. { x ### x does stuff ###, y } /// In practice, these patterns will always be Identifier - RecordDestructure(&'a [Loc>]), + RecordDestructure(Collection<'a, Loc>>), /// A required field pattern, e.g. { x: Just 0 } -> ... /// Can only occur inside of a RecordDestructure diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index ad3b94f7f6..d2f7b24487 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -1,4 +1,6 @@ -use crate::ast::{AssignedField, CommentOrNewline, Def, Expr, Pattern, Spaceable, TypeAnnotation}; +use crate::ast::{ + AssignedField, Collection, CommentOrNewline, Def, Expr, Pattern, Spaceable, TypeAnnotation, +}; use crate::blankspace::{space0_after_e, space0_around_ee, space0_before_e, space0_e}; use crate::ident::{lowercase_ident, parse_ident, Ident}; use crate::keyword; @@ -1446,10 +1448,7 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result expr_to_pattern_help(arena, sub_expr), - Expr::Record { - fields, - final_comments: _, - } => { + Expr::Record(fields) => { let mut loc_patterns = Vec::with_capacity_in(fields.len(), arena); for loc_assigned_field in fields.iter() { @@ -1459,7 +1458,10 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result Ok(Pattern::FloatLiteral(string)), @@ -2312,13 +2314,15 @@ fn record_literal_help<'a>(min_indent: u16) -> impl Parser<'a, Expr<'a>, EExpr<' let mut value = match opt_update { Some(update) => Expr::RecordUpdate { update: &*arena.alloc(update), - fields: loc_assigned_fields_with_comments.value.0.into_bump_slice(), - final_comments: arena.alloc(loc_assigned_fields_with_comments.value.1), + fields: Collection { + items: loc_assigned_fields_with_comments.value.0.into_bump_slice(), + final_comments: arena.alloc(loc_assigned_fields_with_comments.value.1), + }, }, - None => Expr::Record { - fields: loc_assigned_fields_with_comments.value.0.into_bump_slice(), + None => Expr::Record(Collection { + items: loc_assigned_fields_with_comments.value.0.into_bump_slice(), final_comments: loc_assigned_fields_with_comments.value.1, - }, + }), }; // there can be field access, e.g. `{ x : 4 }.x` diff --git a/compiler/parse/src/pattern.rs b/compiler/parse/src/pattern.rs index a855537d4f..6f1dfa3351 100644 --- a/compiler/parse/src/pattern.rs +++ b/compiler/parse/src/pattern.rs @@ -331,10 +331,7 @@ fn record_pattern_help<'a>(min_indent: u16) -> impl Parser<'a, Pattern<'a>, PRec ) .parse(arena, state)?; - // TODO - let _unused = fields.final_comments; - - let result = Pattern::RecordDestructure(fields.items); + let result = Pattern::RecordDestructure(fields); Ok((MadeProgress, result, state)) } diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index cf699dd62b..4e35c1b007 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -460,10 +460,7 @@ mod test_parse { #[test] fn empty_record() { let arena = Bump::new(); - let expected = Record { - fields: &[], - final_comments: &[], - }; + let expected = Record(Collection::empty()); let actual = parse_expr_with(&arena, "{}"); assert_eq!(Ok(expected), actual); @@ -493,8 +490,7 @@ mod test_parse { let update_target = Located::new(0, 0, 2, 13, var); let expected = RecordUpdate { update: &*arena.alloc(update_target), - fields, - final_comments: &(&[] as &[_]), + fields: Collection::with_items(fields), }; let actual = parse_expr_with(&arena, "{ Foo.Bar.baz & x: 5, y: 0 }"); @@ -526,10 +522,7 @@ mod test_parse { Located::new(0, 0, 1, 26, label1), Located::new(0, 0, 28, 32, label2), ]; - let expected = Record { - fields, - final_comments: &[], - }; + let expected = Record(Collection::with_items(fields)); let actual = parse_expr_with(&arena, "{x : if True then 1 else 2, y: 3 }"); assert_eq!(Ok(expected), actual); @@ -1897,7 +1890,13 @@ mod test_parse { Located::new(1, 1, 5, 7, Identifier("y")) ]; let def1 = Def::Body( - arena.alloc(Located::new(1, 1, 0, 8, RecordDestructure(&fields))), + arena.alloc(Located::new( + 1, + 1, + 0, + 8, + RecordDestructure(Collection::with_items(&fields)), + )), arena.alloc(Located::new(1, 1, 11, 12, Num("5"))), ); let loc_def1 = &*arena.alloc(Located::new(1, 1, 0, 12, def1)); @@ -2028,10 +2027,7 @@ mod test_parse { let inner_backpassing = { let identifier_z = Located::new(2, 2, 0, 1, Identifier("z")); - let empty_record = Record { - fields: &[], - final_comments: &[], - }; + let empty_record = Record(Collection::empty()); let loc_empty_record = Located::new(2, 2, 5, 7, empty_record); let var_x = Var { @@ -2950,7 +2946,10 @@ mod test_parse { let arena = Bump::new(); let newlines = &[Newline]; let identifiers1 = &[Located::new(1, 1, 3, 4, Identifier("y"))]; - let pattern1 = Pattern::SpaceBefore(arena.alloc(RecordDestructure(identifiers1)), newlines); + let pattern1 = Pattern::SpaceBefore( + arena.alloc(RecordDestructure(Collection::with_items(identifiers1))), + newlines, + ); let loc_pattern1 = Located::new(1, 1, 1, 6, pattern1); let expr1 = Num("2"); let loc_expr1 = Located::new(1, 1, 10, 11, expr1); @@ -2964,7 +2963,10 @@ mod test_parse { Located::new(2, 2, 3, 4, Identifier("z")), Located::new(2, 2, 6, 7, Identifier("w")), ]; - let pattern2 = Pattern::SpaceBefore(arena.alloc(RecordDestructure(identifiers2)), newlines); + let pattern2 = Pattern::SpaceBefore( + arena.alloc(RecordDestructure(Collection::with_items(identifiers2))), + newlines, + ); let loc_pattern2 = Located::new(2, 2, 1, 9, pattern2); let expr2 = Num("4"); let loc_expr2 = Located::new(2, 2, 13, 14, expr2); From d63405d82483fa87bc7c0b1187c7a19c798599b1 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 08:35:38 -0800 Subject: [PATCH 22/44] Make Expr::List use a Collection --- ast/src/lang/core/expr/expr_to_expr2.rs | 2 +- cli/src/repl/eval.rs | 15 ++-------- compiler/can/src/expr.rs | 4 +-- compiler/can/src/operator.rs | 11 +++----- compiler/fmt/src/expr.rs | 18 ++++-------- compiler/parse/src/ast.rs | 5 +--- compiler/parse/src/expr.rs | 4 +-- compiler/parse/tests/test_parse.rs | 37 ++++++++----------------- 8 files changed, 29 insertions(+), 67 deletions(-) diff --git a/ast/src/lang/core/expr/expr_to_expr2.rs b/ast/src/lang/core/expr/expr_to_expr2.rs index 00fa759f65..d1589a83b6 100644 --- a/ast/src/lang/core/expr/expr_to_expr2.rs +++ b/ast/src/lang/core/expr/expr_to_expr2.rs @@ -132,7 +132,7 @@ pub fn expr_to_expr2<'a>( Str(literal) => flatten_str_literal(env, scope, literal), - List { items, .. } => { + List(items) => { let mut output = Output::default(); let output_ref = &mut output; diff --git a/cli/src/repl/eval.rs b/cli/src/repl/eval.rs index b56200dc6f..1abb0f2f07 100644 --- a/cli/src/repl/eval.rs +++ b/cli/src/repl/eval.rs @@ -133,10 +133,7 @@ fn jit_to_ast_help<'a>( ), Layout::Builtin(Builtin::EmptyList) => { Ok(run_jit_function!(lib, main_fn_name, &'static str, |_| { - Expr::List { - items: &[], - final_comments: &[], - } + Expr::List(Collection::empty()) })) } Layout::Builtin(Builtin::List(elem_layout)) => Ok(run_jit_function!( @@ -431,10 +428,7 @@ fn ptr_to_ast<'a>( num_to_ast(env, number_literal_to_ast(env.arena, num), content) } - Layout::Builtin(Builtin::EmptyList) => Expr::List { - items: &[], - final_comments: &[], - }, + Layout::Builtin(Builtin::EmptyList) => Expr::List(Collection::empty()), Layout::Builtin(Builtin::List(elem_layout)) => { // Turn the (ptr, len) wrapper struct into actual ptr and len values. let len = unsafe { *(ptr.offset(env.ptr_bytes as isize) as *const usize) }; @@ -522,10 +516,7 @@ fn list_to_ast<'a>( let output = output.into_bump_slice(); - Expr::List { - items: output, - final_comments: &[], - } + Expr::List(Collection::with_items(output)) } fn single_tag_union_to_ast<'a>( diff --git a/compiler/can/src/expr.rs b/compiler/can/src/expr.rs index 768bba827e..098ad7926b 100644 --- a/compiler/can/src/expr.rs +++ b/compiler/can/src/expr.rs @@ -303,9 +303,7 @@ pub fn canonicalize_expr<'a>( } } ast::Expr::Str(literal) => flatten_str_literal(env, var_store, scope, literal), - ast::Expr::List { - items: loc_elems, .. - } => { + ast::Expr::List(loc_elems) => { if loc_elems.is_empty() { ( List { diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 4faecdf9bf..98e8a1cba1 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -144,20 +144,17 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a arena.alloc(Located { region, value }) } - List { - items, - final_comments, - } => { + List(items) => { let mut new_items = Vec::with_capacity_in(items.len(), arena); for item in items.iter() { new_items.push(desugar_expr(arena, item)); } let new_items = new_items.into_bump_slice(); - let value: Expr<'a> = List { + let value: Expr<'a> = List(Collection { items: new_items, - final_comments, - }; + final_comments: items.final_comments, + }); arena.alloc(Located { region: loc_expr.region, diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index fd4d43e821..5e028c10ac 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -41,7 +41,7 @@ impl<'a> Formattable<'a> for Expr<'a> { // These expressions always have newlines Defs(_, _) | When(_, _) => true, - List { items, .. } => items.iter().any(|loc_expr| loc_expr.is_multiline()), + List(items) => items.iter().any(|loc_expr| loc_expr.is_multiline()), Str(literal) => { use roc_parse::ast::StrLiteral::*; @@ -290,11 +290,8 @@ impl<'a> Formattable<'a> for Expr<'a> { fmt_if(buf, branches, final_else, self.is_multiline(), indent); } When(loc_condition, branches) => fmt_when(buf, loc_condition, branches, indent), - List { - items, - final_comments, - } => { - fmt_list(buf, items, final_comments, indent); + List(items) => { + fmt_list(buf, *items, indent); } BinOps(lefts, right) => fmt_bin_ops(buf, lefts, right, false, parens, indent), UnaryOp(sub_expr, unary_op) => { @@ -411,12 +408,9 @@ fn fmt_bin_ops<'a>( loc_right_side.format_with_options(buf, apply_needs_parens, Newlines::Yes, indent); } -fn fmt_list<'a>( - buf: &mut String<'a>, - loc_items: &[&Located>], - final_comments: &'a [CommentOrNewline<'a>], - indent: u16, -) { +fn fmt_list<'a>(buf: &mut String<'a>, items: Collection<'a, &Located>>, indent: u16) { + let loc_items = items.items; + let final_comments = items.final_comments; if loc_items.is_empty() && final_comments.iter().all(|c| c.is_newline()) { buf.push_str("[]"); } else { diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index 02fd3acb03..d9661f2f37 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -127,10 +127,7 @@ pub enum Expr<'a> { AccessorFunction(&'a str), // Collection Literals - List { - items: &'a [&'a Loc>], - final_comments: &'a [CommentOrNewline<'a>], - }, + List(Collection<'a, &'a Loc>>), RecordUpdate { update: &'a Loc>, diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index d2f7b24487..ad14d0dfe4 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -2173,10 +2173,10 @@ fn list_literal_help<'a>(min_indent: u16) -> impl Parser<'a, Expr<'a>, EList<'a> allocated.push(parsed_elem); } - let expr = Expr::List { + let expr = Expr::List(Collection { items: allocated.into_bump_slice(), final_comments: elements.final_comments, - }; + }); Ok((MadeProgress, expr, state)) } diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index 4e35c1b007..25d10eb092 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -1166,10 +1166,7 @@ mod test_parse { #[test] fn empty_list() { let arena = Bump::new(); - let expected = List { - items: &[], - final_comments: &[], - }; + let expected = List(Collection::empty()); let actual = parse_expr_with(&arena, "[]"); assert_eq!(Ok(expected), actual); @@ -1179,10 +1176,7 @@ mod test_parse { fn spaces_inside_empty_list() { // This is a regression test! let arena = Bump::new(); - let expected = List { - items: &[], - final_comments: &[], - }; + let expected = List(Collection::empty()); let actual = parse_expr_with(&arena, "[ ]"); assert_eq!(Ok(expected), actual); @@ -1191,10 +1185,10 @@ mod test_parse { #[test] fn newline_inside_empty_list() { let arena = Bump::new(); - let expected = List { + let expected = List(Collection { items: &[], final_comments: &[Newline], - }; + }); let actual = parse_expr_with(&arena, "[\n]"); assert_eq!(Ok(expected), actual); @@ -1203,10 +1197,10 @@ mod test_parse { #[test] fn comment_inside_empty_list() { let arena = Bump::new(); - let expected = List { + let expected = List(Collection { items: &[], final_comments: &[LineComment("comment")], - }; + }); let actual = parse_expr_with(&arena, "[#comment\n]"); assert_eq!(Ok(expected), actual); @@ -1216,10 +1210,7 @@ mod test_parse { fn packed_singleton_list() { let arena = Bump::new(); let items = &[&*arena.alloc(Located::new(0, 0, 1, 2, Num("1")))]; - let expected = List { - items, - final_comments: &[], - }; + let expected = List(Collection::with_items(items)); let actual = parse_expr_with(&arena, "[1]"); assert_eq!(Ok(expected), actual); @@ -1229,10 +1220,7 @@ mod test_parse { fn spaced_singleton_list() { let arena = Bump::new(); let items = &[&*arena.alloc(Located::new(0, 0, 2, 3, Num("1")))]; - let expected = List { - items, - final_comments: &[], - }; + let expected = List(Collection::with_items(items)); let actual = parse_expr_with(&arena, "[ 1 ]"); assert_eq!(Ok(expected), actual); @@ -1247,10 +1235,7 @@ mod test_parse { )); let item = Expr::SpaceBefore(item, arena.alloc([Newline])); let items = [&*arena.alloc(Located::new(1, 1, 0, 1, item))]; - let expected = List { - items: &items, - final_comments: &[], - }; + let expected = List(Collection::with_items(&items)); let actual = parse_expr_with(&arena, "[\n1\n]"); assert_eq!(Ok(expected), actual); @@ -2111,8 +2096,8 @@ mod test_parse { let apply = Expr::Apply( arena.alloc(Located::new(0, 0, 8, 17, var_list_map2)), bumpalo::vec![ in &arena; - &*arena.alloc(Located::new(0, 0, 18, 20, Expr::List{ items: &[], final_comments: &[] })), - &*arena.alloc(Located::new(0, 0, 21, 23, Expr::List{ items: &[], final_comments: &[] })), + &*arena.alloc(Located::new(0, 0, 18, 20, Expr::List(Collection::empty()))), + &*arena.alloc(Located::new(0, 0, 21, 23, Expr::List(Collection::empty()))), ] .into_bump_slice(), CalledVia::Space, From 1fabc64fdf77dd413cdd3012ee7ffcae419aa593 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 10:20:10 -0800 Subject: [PATCH 23/44] Use Collection in Expr::TagUnion --- ast/src/lang/core/types.rs | 2 +- compiler/can/src/annotation.rs | 2 +- compiler/fmt/src/annotation.rs | 49 ++++++++------------------- compiler/load/src/docs.rs | 8 ++--- compiler/parse/src/ast.rs | 3 +- compiler/parse/src/type_annotation.rs | 6 +--- 6 files changed, 21 insertions(+), 49 deletions(-) diff --git a/ast/src/lang/core/types.rs b/ast/src/lang/core/types.rs index 3cb2a03f8f..8cb34c1089 100644 --- a/ast/src/lang/core/types.rs +++ b/ast/src/lang/core/types.rs @@ -428,7 +428,7 @@ pub fn to_type2<'a>( Type2::Record(field_types, ext_type) } TagUnion { tags, ext, .. } => { - let tag_types_vec = can_tags(env, scope, references, tags, region); + let tag_types_vec = can_tags(env, scope, references, tags.items, region); let tag_types = PoolVec::with_capacity(tag_types_vec.len() as u32, env.pool); diff --git a/compiler/can/src/annotation.rs b/compiler/can/src/annotation.rs index 17e0441542..a649bb850c 100644 --- a/compiler/can/src/annotation.rs +++ b/compiler/can/src/annotation.rs @@ -417,7 +417,7 @@ fn can_annotation_help( TagUnion { tags, ext, .. } => { let tag_types = can_tags( env, - tags, + tags.items, region, scope, var_store, diff --git a/compiler/fmt/src/annotation.rs b/compiler/fmt/src/annotation.rs index 4e73d91b7e..41170c7f48 100644 --- a/compiler/fmt/src/annotation.rs +++ b/compiler/fmt/src/annotation.rs @@ -1,6 +1,6 @@ use crate::spaces::{fmt_comments_only, fmt_spaces, newline, NewlineAt, INDENT}; use bumpalo::collections::String; -use roc_parse::ast::{AssignedField, Collection, Expr, Tag, TypeAnnotation}; +use roc_parse::ast::{AssignedField, Expr, Tag, TypeAnnotation}; use roc_region::all::Located; /// Does an AST node need parens around it? @@ -81,9 +81,9 @@ where } macro_rules! format_sequence { - ($buf: expr, $indent:expr, $start:expr, $end:expr, $items:expr, $final_comments:expr, $newline:expr, $t:ident) => { - let is_multiline = - $items.iter().any(|item| item.value.is_multiline()) || !$final_comments.is_empty(); + ($buf: expr, $indent:expr, $start:expr, $end:expr, $items:expr, $newline:expr, $t:ident) => { + let is_multiline = $items.iter().any(|item| item.value.is_multiline()) + || !$items.final_comments.is_empty(); if is_multiline { let braces_indent = $indent + INDENT; @@ -138,7 +138,12 @@ macro_rules! format_sequence { } } } - fmt_comments_only($buf, $final_comments.iter(), NewlineAt::Top, item_indent); + fmt_comments_only( + $buf, + $items.final_comments.iter(), + NewlineAt::Top, + item_indent, + ); newline($buf, braces_indent); $buf.push($end); } else { @@ -192,11 +197,7 @@ impl<'a> Formattable<'a> for TypeAnnotation<'a> { fields.items.iter().any(|field| field.value.is_multiline()) } - TagUnion { - tags, - ext, - final_comments: _, - } => { + TagUnion { tags, ext } => { match ext { Some(ann) if ann.value.is_multiline() => return true, _ => {} @@ -279,36 +280,16 @@ impl<'a> Formattable<'a> for TypeAnnotation<'a> { BoundVariable(v) => buf.push_str(v), Wildcard => buf.push('*'), - TagUnion { - tags, - ext, - final_comments, - } => { - format_sequence!(buf, indent, '[', ']', tags, final_comments, newlines, Tag); + TagUnion { tags, ext } => { + format_sequence!(buf, indent, '[', ']', tags, newlines, Tag); if let Some(loc_ext_ann) = *ext { loc_ext_ann.value.format(buf, indent); } } - Record { - fields: - Collection { - items, - final_comments, - }, - ext, - } => { - format_sequence!( - buf, - indent, - '{', - '}', - items, - final_comments, - newlines, - AssignedField - ); + Record { fields, ext } => { + format_sequence!(buf, indent, '{', '}', fields, newlines, AssignedField); if let Some(loc_ext_ann) = *ext { loc_ext_ann.value.format(buf, indent); diff --git a/compiler/load/src/docs.rs b/compiler/load/src/docs.rs index 0cbc74ea83..edadfbcd5b 100644 --- a/compiler/load/src/docs.rs +++ b/compiler/load/src/docs.rs @@ -235,16 +235,12 @@ fn generate_entry_doc<'a>( fn type_to_docs(in_func_type_ann: bool, type_annotation: ast::TypeAnnotation) -> TypeAnnotation { match type_annotation { - ast::TypeAnnotation::TagUnion { - tags, - ext, - final_comments: _, - } => { + ast::TypeAnnotation::TagUnion { tags, ext } => { let mut tags_to_render: Vec = Vec::new(); let mut any_tags_are_private = false; - for tag in tags { + for tag in tags.iter() { match tag_to_doc(in_func_type_ann, tag.value) { None => { any_tags_are_private = true; diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index d9661f2f37..797c360018 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -266,11 +266,10 @@ pub enum TypeAnnotation<'a> { /// A tag union, e.g. `[ TagUnion { - tags: &'a [Loc>], /// The row type variable in an open tag union, e.g. the `a` in `[ Foo, Bar ]a`. /// This is None if it's a closed tag union like `[ Foo, Bar]`. ext: Option<&'a Loc>>, - final_comments: &'a [CommentOrNewline<'a>], + tags: Collection<'a, Loc>>, }, /// The `*` type variable, e.g. in (List *) diff --git a/compiler/parse/src/type_annotation.rs b/compiler/parse/src/type_annotation.rs index 40628eb622..29065c5ed6 100644 --- a/compiler/parse/src/type_annotation.rs +++ b/compiler/parse/src/type_annotation.rs @@ -40,11 +40,7 @@ fn tag_union_type<'a>(min_indent: u16) -> impl Parser<'a, TypeAnnotation<'a>, ET ))) .parse(arena, state)?; - let result = TypeAnnotation::TagUnion { - tags: tags.items, - ext, - final_comments: tags.final_comments, - }; + let result = TypeAnnotation::TagUnion { tags, ext }; Ok((MadeProgress, result, state)) } From 8d7c252fce4fd33ec54e447b848edc4c62e72b82 Mon Sep 17 00:00:00 2001 From: Theo Felippe Date: Sat, 13 Nov 2021 18:02:58 +0000 Subject: [PATCH 24/44] implemented Str.trimRight --- compiler/builtins/bitcode/src/main.zig | 1 + compiler/builtins/bitcode/src/str.zig | 107 ++++++++++++++++++++++++ compiler/builtins/src/bitcode.rs | 1 + compiler/builtins/src/std.rs | 7 ++ compiler/can/src/builtins.rs | 6 ++ compiler/gen_llvm/src/llvm/build.rs | 7 ++ compiler/gen_llvm/src/llvm/build_str.rs | 10 +++ compiler/gen_wasm/src/low_level.rs | 2 +- compiler/module/src/low_level.rs | 2 + compiler/module/src/symbol.rs | 1 + compiler/mono/src/borrow.rs | 1 + compiler/test_gen/src/gen_str.rs | 93 ++++++++++++++++++++ 12 files changed, 237 insertions(+), 1 deletion(-) diff --git a/compiler/builtins/bitcode/src/main.zig b/compiler/builtins/bitcode/src/main.zig index 374aaa7bc5..c270263b37 100644 --- a/compiler/builtins/bitcode/src/main.zig +++ b/compiler/builtins/bitcode/src/main.zig @@ -127,6 +127,7 @@ comptime { exportStrFn(str.repeat, "repeat"); exportStrFn(str.strTrim, "trim"); exportStrFn(str.strTrimLeft, "trim_left"); + exportStrFn(str.strTrimRight, "trim_right"); inline for (INTEGERS) |T| { str.exportFromInt(T, ROC_BUILTINS ++ "." ++ STR ++ ".from_int."); diff --git a/compiler/builtins/bitcode/src/str.zig b/compiler/builtins/bitcode/src/str.zig index 8eb369b308..474b716faf 100644 --- a/compiler/builtins/bitcode/src/str.zig +++ b/compiler/builtins/bitcode/src/str.zig @@ -1584,6 +1584,42 @@ pub fn strTrimLeft(string: RocStr) callconv(.C) RocStr { return RocStr.empty(); } +pub fn strTrimRight(string: RocStr) callconv(.C) RocStr { + if (string.str_bytes) |bytes_ptr| { + const leading_bytes = countLeadingWhitespaceBytes(string); + const trailing_bytes = countTrailingWhitespaceBytes(string); + const original_len = string.len(); + + if (original_len == trailing_bytes) { + string.deinit(); + return RocStr.empty(); + } + + const new_len = original_len - trailing_bytes; + + const small_or_shared = new_len <= SMALL_STR_MAX_LENGTH or !string.isRefcountOne(); + if (small_or_shared) { + return RocStr.init(string.asU8ptr(), new_len); + } + + // nonempty, large, and unique: + + var i: usize = 0; + while (i < new_len) : (i += 1) { + const dest = bytes_ptr + i; + const source = dest; + @memcpy(dest, source, 1); + } + + var new_string = string; + new_string.str_len = new_len; + + return new_string; + } + + return RocStr.empty(); +} + fn countLeadingWhitespaceBytes(string: RocStr) usize { var byte_count: usize = 0; @@ -1820,6 +1856,77 @@ test "strTrimLeft: small to small" { try expect(trimmed.isSmallStr()); } +test "strTrimRight: empty" { + const trimmedEmpty = strTrimRight(RocStr.empty()); + try expect(trimmedEmpty.eq(RocStr.empty())); +} + +test "strTrimRight: blank" { + const original_bytes = " "; + const original = RocStr.init(original_bytes, original_bytes.len); + defer original.deinit(); + + const trimmed = strTrimRight(original); + + try expect(trimmed.eq(RocStr.empty())); +} + +test "strTrimRight: large to large" { + const original_bytes = " hello giant world "; + const original = RocStr.init(original_bytes, original_bytes.len); + defer original.deinit(); + + try expect(!original.isSmallStr()); + + const expected_bytes = " hello giant world"; + const expected = RocStr.init(expected_bytes, expected_bytes.len); + defer expected.deinit(); + + try expect(!expected.isSmallStr()); + + const trimmed = strTrimRight(original); + + try expect(trimmed.eq(expected)); +} + +test "strTrimRight: large to small" { + const original_bytes = " hello world "; + const original = RocStr.init(original_bytes, original_bytes.len); + defer original.deinit(); + + try expect(!original.isSmallStr()); + + const expected_bytes = " hello world"; + const expected = RocStr.init(expected_bytes, expected_bytes.len); + defer expected.deinit(); + + try expect(expected.isSmallStr()); + + const trimmed = strTrimRight(original); + + try expect(trimmed.eq(expected)); + try expect(trimmed.isSmallStr()); +} + +test "strTrimRight: small to small" { + const original_bytes = " hello world "; + const original = RocStr.init(original_bytes, original_bytes.len); + defer original.deinit(); + + try expect(original.isSmallStr()); + + const expected_bytes = " hello world"; + const expected = RocStr.init(expected_bytes, expected_bytes.len); + defer expected.deinit(); + + try expect(expected.isSmallStr()); + + const trimmed = strTrimRight(original); + + try expect(trimmed.eq(expected)); + try expect(trimmed.isSmallStr()); +} + test "ReverseUtf8View: hello world" { const original_bytes = "hello world"; const expected_bytes = "dlrow olleh"; diff --git a/compiler/builtins/src/bitcode.rs b/compiler/builtins/src/bitcode.rs index f8ad6f920f..5197afebbf 100644 --- a/compiler/builtins/src/bitcode.rs +++ b/compiler/builtins/src/bitcode.rs @@ -149,6 +149,7 @@ pub const STR_FROM_UTF8_RANGE: &str = "roc_builtins.str.from_utf8_range"; pub const STR_REPEAT: &str = "roc_builtins.str.repeat"; pub const STR_TRIM: &str = "roc_builtins.str.trim"; pub const STR_TRIM_LEFT: &str = "roc_builtins.str.trim_left"; +pub const STR_TRIM_RIGHT: &str = "roc_builtins.str.trim_right"; pub const DICT_HASH: &str = "roc_builtins.dict.hash"; pub const DICT_HASH_STR: &str = "roc_builtins.dict.hash_str"; diff --git a/compiler/builtins/src/std.rs b/compiler/builtins/src/std.rs index dab48890e3..0f8d1fe154 100644 --- a/compiler/builtins/src/std.rs +++ b/compiler/builtins/src/std.rs @@ -639,6 +639,13 @@ pub fn types() -> MutMap { Box::new(str_type()) ); + // trimRight : Str -> Str + add_top_level_function_type!( + Symbol::STR_TRIM_RIGHT, + vec![str_type()], + Box::new(str_type()) + ); + // trim : Str -> Str add_top_level_function_type!(Symbol::STR_TRIM, vec![str_type()], Box::new(str_type())); diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index 25c4e9b1d7..924e0f6567 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -69,6 +69,7 @@ pub fn builtin_defs_map(symbol: Symbol, var_store: &mut VarStore) -> Option STR_REPEAT => str_repeat, STR_TRIM => str_trim, STR_TRIM_LEFT => str_trim_left, + STR_TRIM_RIGHT => str_trim_right, LIST_LEN => list_len, LIST_GET => list_get, LIST_SET => list_set, @@ -1295,6 +1296,11 @@ fn str_trim_left(symbol: Symbol, var_store: &mut VarStore) -> Def { lowlevel_1(symbol, LowLevel::StrTrimLeft, var_store) } +/// Str.trimRight : Str -> Str +fn str_trim_right(symbol: Symbol, var_store: &mut VarStore) -> Def { + lowlevel_1(symbol, LowLevel::StrTrimRight, var_store) +} + /// Str.repeat : Str, Nat -> Str fn str_repeat(symbol: Symbol, var_store: &mut VarStore) -> Def { let str_var = var_store.fresh(); diff --git a/compiler/gen_llvm/src/llvm/build.rs b/compiler/gen_llvm/src/llvm/build.rs index dc0499ae67..39c94f1a12 100644 --- a/compiler/gen_llvm/src/llvm/build.rs +++ b/compiler/gen_llvm/src/llvm/build.rs @@ -18,6 +18,7 @@ use crate::llvm::build_str::{ empty_str, str_concat, str_count_graphemes, str_ends_with, str_from_float, str_from_int, str_from_utf8, str_from_utf8_range, str_join_with, str_number_of_bytes, str_repeat, str_split, str_starts_with, str_starts_with_code_point, str_to_utf8, str_trim, str_trim_left, + str_trim_right, }; use crate::llvm::compare::{generic_eq, generic_neq}; use crate::llvm::convert::{ @@ -5070,6 +5071,12 @@ fn run_low_level<'a, 'ctx, 'env>( str_trim_left(env, scope, args[0]) } + StrTrimRight => { + // Str.trim : Str -> Str + debug_assert_eq!(args.len(), 1); + + str_trim_right(env, scope, args[0]) + } ListLen => { // List.len : List * -> Int debug_assert_eq!(args.len(), 1); diff --git a/compiler/gen_llvm/src/llvm/build_str.rs b/compiler/gen_llvm/src/llvm/build_str.rs index e9bd164b0a..975d979866 100644 --- a/compiler/gen_llvm/src/llvm/build_str.rs +++ b/compiler/gen_llvm/src/llvm/build_str.rs @@ -269,6 +269,16 @@ pub fn str_trim_left<'a, 'ctx, 'env>( call_bitcode_fn(env, &[str_i128.into()], bitcode::STR_TRIM_LEFT) } +/// Str.trimRight : Str -> Str +pub fn str_trim_right<'a, 'ctx, 'env>( + env: &Env<'a, 'ctx, 'env>, + scope: &Scope<'a, 'ctx>, + str_symbol: Symbol, +) -> BasicValueEnum<'ctx> { + let str_i128 = str_symbol_to_c_abi(env, scope, str_symbol); + call_bitcode_fn(env, &[str_i128.into()], bitcode::STR_TRIM_RIGHT) +} + /// Str.fromInt : Int -> Str pub fn str_from_int<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, diff --git a/compiler/gen_wasm/src/low_level.rs b/compiler/gen_wasm/src/low_level.rs index 2cf05a7a08..c44387e1dd 100644 --- a/compiler/gen_wasm/src/low_level.rs +++ b/compiler/gen_wasm/src/low_level.rs @@ -28,7 +28,7 @@ pub fn build_call_low_level<'a>( match lowlevel { StrConcat | StrJoinWith | StrIsEmpty | StrStartsWith | StrStartsWithCodePt - | StrEndsWith | StrSplit | StrCountGraphemes | StrFromInt | StrFromUtf8 | StrTrimLeft + | StrEndsWith | StrSplit | StrCountGraphemes | StrFromInt | StrFromUtf8 | StrTrimLeft | StrTrimRight | StrFromUtf8Range | StrToUtf8 | StrRepeat | StrFromFloat | StrTrim | ListLen | ListGetUnsafe | ListSet | ListSingle | ListRepeat | ListReverse | ListConcat | ListContains | ListAppend | ListPrepend | ListJoin | ListRange | ListMap | ListMap2 diff --git a/compiler/module/src/low_level.rs b/compiler/module/src/low_level.rs index dd9a4f020b..bc8d2706d0 100644 --- a/compiler/module/src/low_level.rs +++ b/compiler/module/src/low_level.rs @@ -19,6 +19,7 @@ pub enum LowLevel { StrFromFloat, StrTrim, StrTrimLeft, + StrTrimRight, ListLen, ListGetUnsafe, ListSet, @@ -131,6 +132,7 @@ macro_rules! first_order { | StrRepeat | StrTrim | StrTrimLeft + | StrTrimRight | StrFromFloat | ListLen | ListGetUnsafe diff --git a/compiler/module/src/symbol.rs b/compiler/module/src/symbol.rs index 2233003ca0..44bf51012a 100644 --- a/compiler/module/src/symbol.rs +++ b/compiler/module/src/symbol.rs @@ -1019,6 +1019,7 @@ define_builtins! { 19 STR_REPEAT: "repeat" 20 STR_TRIM: "trim" 21 STR_TRIM_LEFT: "trimLeft" + 22 STR_TRIM_RIGHT: "trimRight" } 4 LIST: "List" => { 0 LIST_LIST: "List" imported // the List.List type alias diff --git a/compiler/mono/src/borrow.rs b/compiler/mono/src/borrow.rs index 045dfb38cb..889272bcf8 100644 --- a/compiler/mono/src/borrow.rs +++ b/compiler/mono/src/borrow.rs @@ -941,6 +941,7 @@ pub fn lowlevel_borrow_signature(arena: &Bump, op: LowLevel) -> &[bool] { StrConcat => arena.alloc_slice_copy(&[owned, borrowed]), StrTrim => arena.alloc_slice_copy(&[owned]), StrTrimLeft => arena.alloc_slice_copy(&[owned]), + StrTrimRight => arena.alloc_slice_copy(&[owned]), StrSplit => arena.alloc_slice_copy(&[borrowed, borrowed]), ListSingle => arena.alloc_slice_copy(&[irrelevant]), ListRepeat => arena.alloc_slice_copy(&[irrelevant, borrowed]), diff --git a/compiler/test_gen/src/gen_str.rs b/compiler/test_gen/src/gen_str.rs index d5949996ee..691ae32971 100644 --- a/compiler/test_gen/src/gen_str.rs +++ b/compiler/test_gen/src/gen_str.rs @@ -1244,3 +1244,96 @@ fn str_trim_left_small_to_small_shared() { (RocStr, RocStr) ); } + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn str_trim_right_small_blank_string() { + assert_evals_to!(indoc!(r#"Str.trimRight " ""#), RocStr::from(""), RocStr); +} + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn str_trim_right_small_to_small() { + assert_evals_to!( + indoc!(r#"Str.trimRight " hello world ""#), + RocStr::from(" hello world"), + RocStr + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn str_trim_right_large_to_large_unique() { + assert_evals_to!( + indoc!(r#"Str.trimRight (Str.concat " hello world from a large string" " ")"#), + RocStr::from(" hello world from a large string"), + RocStr + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn str_trim_right_large_to_small_unique() { + assert_evals_to!( + indoc!(r#"Str.trimRight (Str.concat " hello world" " ")"#), + RocStr::from(" hello world"), + RocStr + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn str_trim_right_large_to_large_shared() { + assert_evals_to!( + indoc!( + r#" + original : Str + original = " hello world world " + + { trimmed: Str.trimRight original, original: original } + "# + ), + ( + RocStr::from(" hello world world "), + RocStr::from(" hello world world"), + ), + (RocStr, RocStr) + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn str_trim_right_large_to_small_shared() { + assert_evals_to!( + indoc!( + r#" + original : Str + original = " hello world " + + { trimmed: Str.trimRight original, original: original } + "# + ), + ( + RocStr::from(" hello world "), + RocStr::from(" hello world"), + ), + (RocStr, RocStr) + ); +} + +#[test] +#[cfg(any(feature = "gen-llvm"))] +fn str_trim_right_small_to_small_shared() { + assert_evals_to!( + indoc!( + r#" + original : Str + original = " hello world " + + { trimmed: Str.trimRight original, original: original } + "# + ), + (RocStr::from(" hello world "), RocStr::from(" hello world"),), + (RocStr, RocStr) + ); +} From ed3ce2962cf2bffafe06d5c3785178e3acc58700 Mon Sep 17 00:00:00 2001 From: Theo Felippe Date: Sat, 13 Nov 2021 18:09:18 +0000 Subject: [PATCH 25/44] removed unused const --- compiler/builtins/bitcode/src/str.zig | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/builtins/bitcode/src/str.zig b/compiler/builtins/bitcode/src/str.zig index 474b716faf..26972282dc 100644 --- a/compiler/builtins/bitcode/src/str.zig +++ b/compiler/builtins/bitcode/src/str.zig @@ -1586,7 +1586,6 @@ pub fn strTrimLeft(string: RocStr) callconv(.C) RocStr { pub fn strTrimRight(string: RocStr) callconv(.C) RocStr { if (string.str_bytes) |bytes_ptr| { - const leading_bytes = countLeadingWhitespaceBytes(string); const trailing_bytes = countTrailingWhitespaceBytes(string); const original_len = string.len(); From 277681fd0647bcaae740a8cc4a5b00289141a6d1 Mon Sep 17 00:00:00 2001 From: Theo Felippe Date: Sat, 13 Nov 2021 18:12:14 +0000 Subject: [PATCH 26/44] fixed formatting --- compiler/gen_wasm/src/low_level.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/gen_wasm/src/low_level.rs b/compiler/gen_wasm/src/low_level.rs index c44387e1dd..7dfbcf3b96 100644 --- a/compiler/gen_wasm/src/low_level.rs +++ b/compiler/gen_wasm/src/low_level.rs @@ -28,15 +28,15 @@ pub fn build_call_low_level<'a>( match lowlevel { StrConcat | StrJoinWith | StrIsEmpty | StrStartsWith | StrStartsWithCodePt - | StrEndsWith | StrSplit | StrCountGraphemes | StrFromInt | StrFromUtf8 | StrTrimLeft | StrTrimRight - | StrFromUtf8Range | StrToUtf8 | StrRepeat | StrFromFloat | StrTrim | ListLen - | ListGetUnsafe | ListSet | ListSingle | ListRepeat | ListReverse | ListConcat - | ListContains | ListAppend | ListPrepend | ListJoin | ListRange | ListMap | ListMap2 - | ListMap3 | ListMap4 | ListMapWithIndex | ListKeepIf | ListWalk | ListWalkUntil - | ListWalkBackwards | ListKeepOks | ListKeepErrs | ListSortWith | ListSublist - | ListDrop | ListDropAt | ListSwap | ListAny | ListFindUnsafe | DictSize | DictEmpty - | DictInsert | DictRemove | DictContains | DictGetUnsafe | DictKeys | DictValues - | DictUnion | DictIntersection | DictDifference | DictWalk | SetFromList => { + | StrEndsWith | StrSplit | StrCountGraphemes | StrFromInt | StrFromUtf8 | StrTrimLeft + | StrTrimRight | StrFromUtf8Range | StrToUtf8 | StrRepeat | StrFromFloat | StrTrim + | ListLen | ListGetUnsafe | ListSet | ListSingle | ListRepeat | ListReverse + | ListConcat | ListContains | ListAppend | ListPrepend | ListJoin | ListRange | ListMap + | ListMap2 | ListMap3 | ListMap4 | ListMapWithIndex | ListKeepIf | ListWalk + | ListWalkUntil | ListWalkBackwards | ListKeepOks | ListKeepErrs | ListSortWith + | ListSublist | ListDrop | ListDropAt | ListSwap | ListAny | ListFindUnsafe | DictSize + | DictEmpty | DictInsert | DictRemove | DictContains | DictGetUnsafe | DictKeys + | DictValues | DictUnion | DictIntersection | DictDifference | DictWalk | SetFromList => { return NotImplemented; } From 9bf16749463105e746ab4b04c056edfaec0f560f Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Sat, 13 Nov 2021 16:06:12 -0800 Subject: [PATCH 27/44] Shrink Collection to make parse_expr_size test pass --- compiler/can/src/operator.rs | 51 +++------ compiler/fmt/src/annotation.rs | 4 +- compiler/fmt/src/expr.rs | 6 +- compiler/parse/src/ast.rs | 143 +++++++++++++++++++------- compiler/parse/src/expr.rs | 44 +++----- compiler/parse/src/parser.rs | 8 +- compiler/parse/src/type_annotation.rs | 10 +- compiler/parse/tests/test_parse.rs | 14 ++- 8 files changed, 155 insertions(+), 125 deletions(-) diff --git a/compiler/can/src/operator.rs b/compiler/can/src/operator.rs index 98e8a1cba1..c11242d71c 100644 --- a/compiler/can/src/operator.rs +++ b/compiler/can/src/operator.rs @@ -6,7 +6,7 @@ use roc_module::ident::ModuleName; use roc_module::operator::BinOp::Pizza; use roc_module::operator::{BinOp, CalledVia}; use roc_parse::ast::Expr::{self, *}; -use roc_parse::ast::{AssignedField, Collection, Def, WhenBranch}; +use roc_parse::ast::{AssignedField, Def, WhenBranch}; use roc_region::all::{Located, Region}; // BinOp precedence logic adapted from Gluon by Markus Westerlind @@ -151,62 +151,39 @@ pub fn desugar_expr<'a>(arena: &'a Bump, loc_expr: &'a Located>) -> &'a new_items.push(desugar_expr(arena, item)); } let new_items = new_items.into_bump_slice(); - let value: Expr<'a> = List(Collection { - items: new_items, - final_comments: items.final_comments, - }); + let value: Expr<'a> = List(items.replace_items(new_items)); arena.alloc(Located { region: loc_expr.region, value, }) } - Record(fields) => { - let mut new_fields = Vec::with_capacity_in(fields.len(), arena); - - for field in fields.iter() { + Record(fields) => arena.alloc(Located { + region: loc_expr.region, + value: Record(fields.map_items(arena, |field| { let value = desugar_field(arena, &field.value); - - new_fields.push(Located { + Located { value, region: field.region, - }); - } - - let new_fields = new_fields.into_bump_slice(); - - arena.alloc(Located { - region: loc_expr.region, - value: Record(Collection { - items: new_fields, - final_comments: fields.final_comments, - }), - }) - } + } + })), + }), RecordUpdate { fields, update } => { // NOTE the `update` field is always a `Var { .. }` and does not need to be desugared - let mut new_fields = Vec::with_capacity_in(fields.len(), arena); - - for field in fields.iter() { + let new_fields = fields.map_items(arena, |field| { let value = desugar_field(arena, &field.value); - - new_fields.push(Located { + Located { value, region: field.region, - }); - } - - let new_fields = new_fields.into_bump_slice(); + } + }); arena.alloc(Located { region: loc_expr.region, value: RecordUpdate { update: *update, - fields: Collection { - items: new_fields, - final_comments: fields.final_comments, - }, + fields: new_fields, }, }) } diff --git a/compiler/fmt/src/annotation.rs b/compiler/fmt/src/annotation.rs index 41170c7f48..c74636dfa9 100644 --- a/compiler/fmt/src/annotation.rs +++ b/compiler/fmt/src/annotation.rs @@ -83,7 +83,7 @@ where macro_rules! format_sequence { ($buf: expr, $indent:expr, $start:expr, $end:expr, $items:expr, $newline:expr, $t:ident) => { let is_multiline = $items.iter().any(|item| item.value.is_multiline()) - || !$items.final_comments.is_empty(); + || !$items.final_comments().is_empty(); if is_multiline { let braces_indent = $indent + INDENT; @@ -140,7 +140,7 @@ macro_rules! format_sequence { } fmt_comments_only( $buf, - $items.final_comments.iter(), + $items.final_comments().iter(), NewlineAt::Top, item_indent, ); diff --git a/compiler/fmt/src/expr.rs b/compiler/fmt/src/expr.rs index 5e028c10ac..f82fd61a1f 100644 --- a/compiler/fmt/src/expr.rs +++ b/compiler/fmt/src/expr.rs @@ -408,9 +408,9 @@ fn fmt_bin_ops<'a>( loc_right_side.format_with_options(buf, apply_needs_parens, Newlines::Yes, indent); } -fn fmt_list<'a>(buf: &mut String<'a>, items: Collection<'a, &Located>>, indent: u16) { +fn fmt_list<'a>(buf: &mut String<'a>, items: Collection<'a, &'a Located>>, indent: u16) { let loc_items = items.items; - let final_comments = items.final_comments; + let final_comments = items.final_comments(); if loc_items.is_empty() && final_comments.iter().all(|c| c.is_newline()) { buf.push_str("[]"); } else { @@ -910,7 +910,7 @@ fn fmt_record<'a>( indent: u16, ) { let loc_fields = fields.items; - let final_comments = fields.final_comments; + let final_comments = fields.final_comments(); if loc_fields.is_empty() && final_comments.iter().all(|c| c.is_newline()) { buf.push_str("{}"); } else { diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index 797c360018..61ce017865 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -1,44 +1,10 @@ use crate::header::{AppHeader, ImportsEntry, InterfaceHeader, PlatformHeader, TypedIdent}; use crate::ident::Ident; -use bumpalo::collections::String; +use bumpalo::collections::{String, Vec}; use bumpalo::Bump; use roc_module::operator::{BinOp, CalledVia, UnaryOp}; use roc_region::all::{Loc, Position, Region}; -#[derive(Copy, Clone, Debug, PartialEq)] -pub struct Collection<'a, T> { - pub items: &'a [T], - pub final_comments: &'a [CommentOrNewline<'a>], -} - -impl<'a, T> Collection<'a, T> { - pub fn empty() -> Collection<'a, T> { - Collection { - items: &[], - final_comments: &[], - } - } - - pub fn with_items(items: &'a [T]) -> Collection<'a, T> { - Collection { - items, - final_comments: &[], - } - } - - pub fn iter(&self) -> impl Iterator { - self.items.iter() - } - - pub fn len(&self) -> usize { - self.items.len() - } - - pub fn is_empty(&self) -> bool { - self.items.is_empty() - } -} - #[derive(Clone, Debug, PartialEq)] pub enum Module<'a> { Interface { header: InterfaceHeader<'a> }, @@ -523,6 +489,113 @@ impl<'a> Pattern<'a> { } } } +#[derive(Copy, Clone, Debug)] +pub struct Collection<'a, T> { + pub items: &'a [T], + // Use a pointer to a slice (rather than just a slice), in order to avoid bloating + // Ast variants. The final_comments field is rarely accessed in the hot path, so + // this shouldn't matter much for perf. + // Use an Option, so it's possible to initialize without allocating. + final_comments: Option<&'a &'a [CommentOrNewline<'a>]>, +} + +impl<'a, T> Collection<'a, T> { + pub fn empty() -> Collection<'a, T> { + Collection { + items: &[], + final_comments: None, + } + } + + pub fn with_items(items: &'a [T]) -> Collection<'a, T> { + Collection { + items, + final_comments: None, + } + } + + pub fn with_items_and_comments( + arena: &'a Bump, + items: &'a [T], + comments: &'a [CommentOrNewline<'a>], + ) -> Collection<'a, T> { + Collection { + items, + final_comments: if comments.is_empty() { + None + } else { + Some(arena.alloc(comments)) + }, + } + } + + pub fn replace_items(&self, new_items: &'a [V]) -> Collection<'a, V> { + Collection { + items: new_items, + final_comments: self.final_comments, + } + } + + pub fn ptrify_items(&self, arena: &'a Bump) -> Collection<'a, &'a T> { + let mut allocated = Vec::with_capacity_in(self.len(), arena); + + for parsed_elem in self.items { + allocated.push(parsed_elem); + } + + self.replace_items(allocated.into_bump_slice()) + } + + pub fn map_items(&self, arena: &'a Bump, f: impl Fn(&'a T) -> V) -> Collection<'a, V> { + let mut allocated = Vec::with_capacity_in(self.len(), arena); + + for parsed_elem in self.items { + allocated.push(f(parsed_elem)); + } + + self.replace_items(allocated.into_bump_slice()) + } + + pub fn map_items_result( + &self, + arena: &'a Bump, + f: impl Fn(&T) -> Result, + ) -> Result, E> { + let mut allocated = Vec::with_capacity_in(self.len(), arena); + + for parsed_elem in self.items { + allocated.push(f(parsed_elem)?); + } + + Ok(self.replace_items(allocated.into_bump_slice())) + } + + pub fn final_comments(&self) -> &'a [CommentOrNewline<'a>] { + if let Some(final_comments) = self.final_comments { + *final_comments + } else { + &[] + } + } + + pub fn iter(&self) -> impl Iterator { + self.items.iter() + } + + pub fn len(&self) -> usize { + self.items.len() + } + + pub fn is_empty(&self) -> bool { + self.items.is_empty() + } +} + +impl<'a, T: PartialEq> PartialEq for Collection<'a, T> { + fn eq(&self, other: &Self) -> bool { + self.items == other.items && self.final_comments() == other.final_comments() + } +} pub trait Spaceable<'a> { fn before(&'a self, _: &'a [CommentOrNewline<'a>]) -> Self; diff --git a/compiler/parse/src/expr.rs b/compiler/parse/src/expr.rs index ad14d0dfe4..90eaeb268f 100644 --- a/compiler/parse/src/expr.rs +++ b/compiler/parse/src/expr.rs @@ -1449,19 +1449,13 @@ fn expr_to_pattern_help<'a>(arena: &'a Bump, expr: &Expr<'a>) -> Result expr_to_pattern_help(arena, sub_expr), Expr::Record(fields) => { - let mut loc_patterns = Vec::with_capacity_in(fields.len(), arena); - - for loc_assigned_field in fields.iter() { + let patterns = fields.map_items_result(arena, |loc_assigned_field| { let region = loc_assigned_field.region; let value = assigned_expr_field_to_pattern_help(arena, &loc_assigned_field.value)?; + Ok(Located { region, value }) + })?; - loc_patterns.push(Located { region, value }); - } - - Ok(Pattern::RecordDestructure(Collection { - items: loc_patterns.into_bump_slice(), - final_comments: fields.final_comments, - })) + Ok(Pattern::RecordDestructure(patterns)) } Expr::Float(string) => Ok(Pattern::FloatLiteral(string)), @@ -2167,16 +2161,8 @@ fn list_literal_help<'a>(min_indent: u16) -> impl Parser<'a, Expr<'a>, EList<'a> ) .parse(arena, state)?; - let mut allocated = Vec::with_capacity_in(elements.items.len(), arena); - - for parsed_elem in elements.items { - allocated.push(parsed_elem); - } - - let expr = Expr::List(Collection { - items: allocated.into_bump_slice(), - final_comments: elements.final_comments, - }); + let elements = elements.ptrify_items(arena); + let expr = Expr::List(elements); Ok((MadeProgress, expr, state)) } @@ -2314,15 +2300,17 @@ fn record_literal_help<'a>(min_indent: u16) -> impl Parser<'a, Expr<'a>, EExpr<' let mut value = match opt_update { Some(update) => Expr::RecordUpdate { update: &*arena.alloc(update), - fields: Collection { - items: loc_assigned_fields_with_comments.value.0.into_bump_slice(), - final_comments: arena.alloc(loc_assigned_fields_with_comments.value.1), - }, + fields: Collection::with_items_and_comments( + arena, + loc_assigned_fields_with_comments.value.0.into_bump_slice(), + arena.alloc(loc_assigned_fields_with_comments.value.1), + ), }, - None => Expr::Record(Collection { - items: loc_assigned_fields_with_comments.value.0.into_bump_slice(), - final_comments: loc_assigned_fields_with_comments.value.1, - }), + None => Expr::Record(Collection::with_items_and_comments( + arena, + loc_assigned_fields_with_comments.value.0.into_bump_slice(), + loc_assigned_fields_with_comments.value.1, + )), }; // there can be field access, e.g. `{ x : 4 }.x` diff --git a/compiler/parse/src/parser.rs b/compiler/parse/src/parser.rs index c1e0187b29..07b9800359 100644 --- a/compiler/parse/src/parser.rs +++ b/compiler/parse/src/parser.rs @@ -1300,10 +1300,10 @@ macro_rules! collection_trailing_sep_e { } } - let collection = $crate::ast::Collection { - items: parsed_elems.into_bump_slice(), - final_comments, - }; + let collection = $crate::ast::Collection::with_items_and_comments( + arena, + parsed_elems.into_bump_slice(), + final_comments); Ok((MadeProgress, collection, state)) } diff --git a/compiler/parse/src/type_annotation.rs b/compiler/parse/src/type_annotation.rs index 29065c5ed6..64b6c69fb0 100644 --- a/compiler/parse/src/type_annotation.rs +++ b/compiler/parse/src/type_annotation.rs @@ -1,4 +1,4 @@ -use crate::ast::{AssignedField, Collection, Tag, TypeAnnotation}; +use crate::ast::{AssignedField, Tag, TypeAnnotation}; use crate::blankspace::{space0_around_ee, space0_before_e, space0_e}; use crate::keyword; use crate::parser::{ @@ -291,13 +291,7 @@ fn record_type<'a>(min_indent: u16) -> impl Parser<'a, TypeAnnotation<'a>, EType let field_term = specialize_ref(ETypeRecord::Type, term(min_indent)); let (_, ext, state) = optional(allocated(field_term)).parse(arena, state)?; - let result = Record { - fields: Collection { - items: fields.items, - final_comments: fields.final_comments, - }, - ext, - }; + let result = Record { fields, ext }; Ok((MadeProgress, result, state)) } diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index 25d10eb092..c49820045b 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -1185,10 +1185,7 @@ mod test_parse { #[test] fn newline_inside_empty_list() { let arena = Bump::new(); - let expected = List(Collection { - items: &[], - final_comments: &[Newline], - }); + let expected = List(Collection::with_items_and_comments(&arena, &[], &[Newline])); let actual = parse_expr_with(&arena, "[\n]"); assert_eq!(Ok(expected), actual); @@ -1197,10 +1194,11 @@ mod test_parse { #[test] fn comment_inside_empty_list() { let arena = Bump::new(); - let expected = List(Collection { - items: &[], - final_comments: &[LineComment("comment")], - }); + let expected = List(Collection::with_items_and_comments( + &arena, + &[], + &[LineComment("comment")], + )); let actual = parse_expr_with(&arena, "[#comment\n]"); assert_eq!(Ok(expected), actual); From 3ff3695758f02ef9b5ff2c770bda232d9c4036b7 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Sat, 13 Nov 2021 16:24:35 -0800 Subject: [PATCH 28/44] Add a manual Debug formatter for Collection to make debugging nicer --- compiler/parse/src/ast.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/compiler/parse/src/ast.rs b/compiler/parse/src/ast.rs index 61ce017865..4b79be508e 100644 --- a/compiler/parse/src/ast.rs +++ b/compiler/parse/src/ast.rs @@ -1,3 +1,5 @@ +use std::fmt::Debug; + use crate::header::{AppHeader, ImportsEntry, InterfaceHeader, PlatformHeader, TypedIdent}; use crate::ident::Ident; use bumpalo::collections::{String, Vec}; @@ -489,7 +491,7 @@ impl<'a> Pattern<'a> { } } } -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone)] pub struct Collection<'a, T> { pub items: &'a [T], // Use a pointer to a slice (rather than just a slice), in order to avoid bloating @@ -597,6 +599,19 @@ impl<'a, T: PartialEq> PartialEq for Collection<'a, T> { } } +impl<'a, T: Debug> Debug for Collection<'a, T> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if self.final_comments().is_empty() { + f.debug_list().entries(self.items.iter()).finish() + } else { + f.debug_struct("Collection") + .field("items", &self.items) + .field("final_comments", &self.final_comments()) + .finish() + } + } +} + pub trait Spaceable<'a> { fn before(&'a self, _: &'a [CommentOrNewline<'a>]) -> Self; fn after(&'a self, _: &'a [CommentOrNewline<'a>]) -> Self; From 39263b0ab118846cbcb4c463375b699f24a187a1 Mon Sep 17 00:00:00 2001 From: Brian Carroll Date: Sun, 14 Nov 2021 09:08:19 +0000 Subject: [PATCH 29/44] Shorter name: VmSymbolState --- compiler/gen_wasm/src/storage.rs | 8 ++++---- compiler/gen_wasm/src/wasm_module/code_builder.rs | 12 ++++++------ compiler/gen_wasm/src/wasm_module/mod.rs | 4 +--- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/compiler/gen_wasm/src/storage.rs b/compiler/gen_wasm/src/storage.rs index f99beea619..f552d59c59 100644 --- a/compiler/gen_wasm/src/storage.rs +++ b/compiler/gen_wasm/src/storage.rs @@ -5,7 +5,7 @@ use roc_collections::all::MutMap; use roc_module::symbol::Symbol; use crate::layout::WasmLayout; -use crate::wasm_module::{CodeBuilder, LocalId, ValueType, VirtualMachineSymbolState}; +use crate::wasm_module::{CodeBuilder, LocalId, ValueType, VmSymbolState}; use crate::{copy_memory, round_up_to_alignment, CopyMemoryConfig, PTR_SIZE, PTR_TYPE}; pub enum StoredValueKind { @@ -33,7 +33,7 @@ impl StackMemoryLocation { pub enum StoredValue { /// A value stored implicitly in the VM stack (primitives only) VirtualMachineStack { - vm_state: VirtualMachineSymbolState, + vm_state: VmSymbolState, value_type: ValueType, size: u32, }, @@ -126,7 +126,7 @@ impl<'a> Storage<'a> { } } _ => StoredValue::VirtualMachineStack { - vm_state: VirtualMachineSymbolState::NotYetPushed, + vm_state: VmSymbolState::NotYetPushed, value_type: *value_type, size: *size, }, @@ -422,7 +422,7 @@ impl<'a> Storage<'a> { } = storage { let local_id = self.get_next_local_id(); - if vm_state != VirtualMachineSymbolState::NotYetPushed { + if vm_state != VmSymbolState::NotYetPushed { code_builder.load_symbol(symbol, vm_state, local_id); code_builder.set_local(local_id); } diff --git a/compiler/gen_wasm/src/wasm_module/code_builder.rs b/compiler/gen_wasm/src/wasm_module/code_builder.rs index 71461111e6..1fe64dc6be 100644 --- a/compiler/gen_wasm/src/wasm_module/code_builder.rs +++ b/compiler/gen_wasm/src/wasm_module/code_builder.rs @@ -73,7 +73,7 @@ impl From for Align { } #[derive(Debug, Clone, PartialEq, Copy)] -pub enum VirtualMachineSymbolState { +pub enum VmSymbolState { /// Value doesn't exist yet NotYetPushed, @@ -169,7 +169,7 @@ impl<'a> CodeBuilder<'a> { /// Set the Symbol that is at the top of the VM stack right now /// We will use this later when we need to load the Symbol - pub fn set_top_symbol(&mut self, sym: Symbol) -> VirtualMachineSymbolState { + pub fn set_top_symbol(&mut self, sym: Symbol) -> VmSymbolState { let len = self.vm_stack.len(); let pushed_at = self.code.len(); @@ -182,7 +182,7 @@ impl<'a> CodeBuilder<'a> { self.vm_stack[len - 1] = sym; - VirtualMachineSymbolState::Pushed { pushed_at } + VmSymbolState::Pushed { pushed_at } } /// Verify if a sequence of symbols is at the top of the stack @@ -227,10 +227,10 @@ impl<'a> CodeBuilder<'a> { pub fn load_symbol( &mut self, symbol: Symbol, - vm_state: VirtualMachineSymbolState, + vm_state: VmSymbolState, next_local_id: LocalId, - ) -> Option { - use VirtualMachineSymbolState::*; + ) -> Option { + use VmSymbolState::*; match vm_state { NotYetPushed => panic!("Symbol {:?} has no value yet. Nothing to load.", symbol), diff --git a/compiler/gen_wasm/src/wasm_module/mod.rs b/compiler/gen_wasm/src/wasm_module/mod.rs index b925bfafd7..2a404e9b48 100644 --- a/compiler/gen_wasm/src/wasm_module/mod.rs +++ b/compiler/gen_wasm/src/wasm_module/mod.rs @@ -4,8 +4,6 @@ pub mod opcodes; pub mod sections; pub mod serialize; -pub use code_builder::{ - Align, BlockType, CodeBuilder, LocalId, ValueType, VirtualMachineSymbolState, -}; +pub use code_builder::{Align, BlockType, CodeBuilder, LocalId, ValueType, VmSymbolState}; pub use linking::{LinkingSubSection, SymInfo}; pub use sections::{ConstExpr, Export, ExportType, Global, GlobalType, Signature, WasmModule}; From a2abf9c3d20b46dc0e985acc17927defaa25b953 Mon Sep 17 00:00:00 2001 From: Brian Carroll Date: Sun, 14 Nov 2021 10:59:32 +0000 Subject: [PATCH 30/44] More accurate model of the Wasm VM's stack machine, with control flow blocks --- .../gen_wasm/src/wasm_module/code_builder.rs | 267 +++++++++++++----- compiler/gen_wasm/src/wasm_module/opcodes.rs | 2 +- compiler/test_gen/src/gen_primitives.rs | 2 +- 3 files changed, 205 insertions(+), 66 deletions(-) diff --git a/compiler/gen_wasm/src/wasm_module/code_builder.rs b/compiler/gen_wasm/src/wasm_module/code_builder.rs index 1fe64dc6be..ed2a850576 100644 --- a/compiler/gen_wasm/src/wasm_module/code_builder.rs +++ b/compiler/gen_wasm/src/wasm_module/code_builder.rs @@ -1,7 +1,6 @@ use bumpalo::collections::vec::Vec; use bumpalo::Bump; use core::panic; -use std::fmt::Debug; use roc_module::symbol::Symbol; @@ -10,6 +9,13 @@ use super::opcodes::{OpCode, OpCode::*}; use super::serialize::{SerialBuffer, Serialize}; use crate::{round_up_to_alignment, FRAME_ALIGNMENT_BYTES, STACK_POINTER_GLOBAL_ID}; +const ENABLE_DEBUG_LOG: bool = true; +macro_rules! log_instruction { + ($($x: expr),+) => { + if ENABLE_DEBUG_LOG { println!($($x,)*); } + }; +} + #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct LocalId(pub u32); @@ -29,6 +35,7 @@ impl Serialize for ValueType { } } +#[derive(PartialEq, Eq, Debug)] pub enum BlockType { NoResult, Value(ValueType), @@ -43,6 +50,24 @@ impl BlockType { } } +/// A control block in our model of the VM +/// Child blocks cannot "see" values from their parent block +struct VmBlock<'a> { + /// opcode indicating what kind of block this is + opcode: OpCode, + /// the stack of values for this block + value_stack: Vec<'a, Symbol>, + /// whether this block pushes a result value to its parent + has_result: bool, +} + +impl std::fmt::Debug for VmBlock<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let result = if self.has_result { "Result" } else { "NoResult" }; + f.write_fmt(format_args!("{:?} {}", self.opcode, result)) + } +} + /// Wasm memory alignment. (Rust representation matches Wasm encoding) #[repr(u8)] #[derive(Clone, Copy, Debug)] @@ -113,6 +138,8 @@ macro_rules! instruction_memargs { #[derive(Debug)] pub struct CodeBuilder<'a> { + arena: &'a Bump, + /// The main container for the instructions code: Vec<'a, u8>, @@ -135,8 +162,8 @@ pub struct CodeBuilder<'a> { inner_length: Vec<'a, u8>, /// Our simulation model of the Wasm stack machine - /// Keeps track of where Symbol values are in the VM stack - vm_stack: Vec<'a, Symbol>, + /// Nested blocks of instructions. A child block can't "see" the stack of its parent block + vm_block_stack: Vec<'a, VmBlock<'a>>, /// Linker info to help combine the Roc module with builtin & platform modules, /// e.g. to modify call instructions when function indices change @@ -146,13 +173,22 @@ pub struct CodeBuilder<'a> { #[allow(clippy::new_without_default)] impl<'a> CodeBuilder<'a> { pub fn new(arena: &'a Bump) -> Self { + let mut vm_block_stack = Vec::with_capacity_in(8, arena); + let function_block = VmBlock { + opcode: BLOCK, + has_result: true, + value_stack: Vec::with_capacity_in(8, arena), + }; + vm_block_stack.push(function_block); + CodeBuilder { + arena, code: Vec::with_capacity_in(1024, arena), insertions: Vec::with_capacity_in(32, arena), insert_bytes: Vec::with_capacity_in(64, arena), preamble: Vec::with_capacity_in(32, arena), inner_length: Vec::with_capacity_in(5, arena), - vm_stack: Vec::with_capacity_in(32, arena), + vm_block_stack, relocations: Vec::with_capacity_in(32, arena), } } @@ -167,35 +203,39 @@ impl<'a> CodeBuilder<'a> { ***********************************************************/ + fn current_stack(&self) -> &Vec<'a, Symbol> { + let block = self.vm_block_stack.last().unwrap(); + &block.value_stack + } + + fn current_stack_mut(&mut self) -> &mut Vec<'a, Symbol> { + let block = self.vm_block_stack.last_mut().unwrap(); + &mut block.value_stack + } + /// Set the Symbol that is at the top of the VM stack right now /// We will use this later when we need to load the Symbol pub fn set_top_symbol(&mut self, sym: Symbol) -> VmSymbolState { - let len = self.vm_stack.len(); + let current_stack = &mut self.vm_block_stack.last_mut().unwrap().value_stack; let pushed_at = self.code.len(); - - if len == 0 { - panic!( - "trying to set symbol with nothing on stack, code = {:?}", - self.code - ); - } - - self.vm_stack[len - 1] = sym; + let top_symbol: &mut Symbol = current_stack.last_mut().unwrap(); + *top_symbol = sym; VmSymbolState::Pushed { pushed_at } } /// Verify if a sequence of symbols is at the top of the stack pub fn verify_stack_match(&self, symbols: &[Symbol]) -> bool { + let current_stack = self.current_stack(); let n_symbols = symbols.len(); - let stack_depth = self.vm_stack.len(); + let stack_depth = current_stack.len(); if n_symbols > stack_depth { return false; } let offset = stack_depth - n_symbols; for (i, sym) in symbols.iter().enumerate() { - if self.vm_stack[offset + i] != *sym { + if current_stack[offset + i] != *sym { return false; } } @@ -214,7 +254,12 @@ impl<'a> CodeBuilder<'a> { end: self.insert_bytes.len(), }); - // println!("insert {:?} {} at byte offset {} ", opcode, immediate, insert_at); + log_instruction!( + "**insert {:?} {} at byte offset {}**", + opcode, + immediate, + insert_at + ); } /// Load a Symbol that is stored in the VM stack @@ -233,35 +278,47 @@ impl<'a> CodeBuilder<'a> { use VmSymbolState::*; match vm_state { - NotYetPushed => panic!("Symbol {:?} has no value yet. Nothing to load.", symbol), + NotYetPushed => unreachable!("Symbol {:?} has no value yet. Nothing to load.", symbol), Pushed { pushed_at } => { - let &top = self.vm_stack.last().unwrap(); - if top == symbol { - // We're lucky, the symbol is already on top of the VM stack - // No code to generate! (This reduces code size by up to 25% in tests.) - // Just let the caller know what happened - Some(Popped { pushed_at }) - } else { - // Symbol is not on top of the stack. Find it. - if let Some(found_index) = self.vm_stack.iter().rposition(|&s| s == symbol) { - // Insert a local.set where the value was created - self.add_insertion(pushed_at, SETLOCAL, next_local_id.0); + match self.current_stack().last() { + Some(top_symbol) if *top_symbol == symbol => { + // We're lucky, the symbol is already on top of the current block's stack. + // No code to generate! (This reduces code size by up to 25% in tests.) + // Just let the caller know what happened + Some(Popped { pushed_at }) + } + _ => { + // Symbol is not on top of the stack. + // We should have saved it to a local, so go back and do that now. - // Take the value out of the stack where local.set was inserted - self.vm_stack.remove(found_index); + // It should still be on the stack in the block where it was assigned. Remove it. + let mut found = false; + for block in self.vm_block_stack.iter_mut() { + if let Some(found_index) = + block.value_stack.iter().position(|&s| s == symbol) + { + block.value_stack.remove(found_index); + found = true; + } + } - // Insert a local.get at the current position + // Go back to the code position where it was pushed, and save it to a local + if found { + self.add_insertion(pushed_at, SETLOCAL, next_local_id.0); + } else { + if ENABLE_DEBUG_LOG { + println!("{:?} has been popped implicitly. Leaving it on the stack.", symbol); + } + self.add_insertion(pushed_at, TEELOCAL, next_local_id.0); + } + + // Recover the value again at the current position self.get_local(next_local_id); self.set_top_symbol(symbol); // This Symbol is no longer stored in the VM stack, but in a local None - } else { - panic!( - "{:?} has state {:?} but not found in VM stack", - symbol, vm_state - ); } } } @@ -284,7 +341,7 @@ impl<'a> CodeBuilder<'a> { /********************************************************** - FINALIZE AND SERIALIZE + FUNCTION HEADER ***********************************************************/ @@ -377,6 +434,12 @@ impl<'a> CodeBuilder<'a> { self.insertions.sort_by_key(|ins| ins.at); } + /********************************************************** + + SERIALIZE + + ***********************************************************/ + /// Serialize all byte vectors in the right order /// Also update relocation offsets relative to the base offset (code section body start) pub fn serialize_with_relocs( @@ -435,33 +498,68 @@ impl<'a> CodeBuilder<'a> { /// Base method for generating instructions /// Emits the opcode and simulates VM stack push/pop - fn inst(&mut self, opcode: OpCode, pops: usize, push: bool) { - let new_len = self.vm_stack.len() - pops as usize; - self.vm_stack.truncate(new_len); + fn inst_base(&mut self, opcode: OpCode, pops: usize, push: bool) { + let current_stack = self.current_stack_mut(); + let new_len = current_stack.len() - pops as usize; + current_stack.truncate(new_len); if push { - self.vm_stack.push(Symbol::WASM_TMP); + current_stack.push(Symbol::WASM_TMP); } - self.code.push(opcode as u8); - - // println!("{:10}\t{:?}", format!("{:?}", opcode), &self.vm_stack); } - fn inst_imm8(&mut self, opcode: OpCode, pops: usize, push: bool, immediate: u8) { - self.inst(opcode, pops, push); - self.code.push(immediate); + /// Plain instruction without any immediates + fn inst(&mut self, opcode: OpCode, pops: usize, push: bool) { + self.inst_base(opcode, pops, push); + log_instruction!( + "{:10}\t\t{:?}", + format!("{:?}", opcode), + self.current_stack() + ); } - // public for use in test code - pub fn inst_imm32(&mut self, opcode: OpCode, pops: usize, push: bool, immediate: u32) { - self.inst(opcode, pops, push); + /// Block instruction + fn inst_block(&mut self, opcode: OpCode, pops: usize, block_type: BlockType) { + self.inst_base(opcode, pops, false); + self.code.push(block_type.as_byte()); + + // Start a new block with a fresh value stack + self.vm_block_stack.push(VmBlock { + opcode, + value_stack: Vec::with_capacity_in(8, self.arena), + has_result: block_type != BlockType::NoResult, + }); + + log_instruction!( + "{:10} {:?}\t{:?}", + format!("{:?}", opcode), + block_type, + &self.vm_block_stack + ); + } + + fn inst_imm32(&mut self, opcode: OpCode, pops: usize, push: bool, immediate: u32) { + self.inst_base(opcode, pops, push); self.code.encode_u32(immediate); + log_instruction!( + "{:10}\t{}\t{:?}", + format!("{:?}", opcode), + immediate, + self.current_stack() + ); } fn inst_mem(&mut self, opcode: OpCode, pops: usize, push: bool, align: Align, offset: u32) { - self.inst(opcode, pops, push); + self.inst_base(opcode, pops, push); self.code.push(align as u8); self.code.encode_u32(offset); + log_instruction!( + "{:10} {:?} {}\t{:?}", + format!("{:?}", opcode), + align, + offset, + self.current_stack() + ); } /// Insert a linker relocation for a memory address @@ -488,22 +586,38 @@ impl<'a> CodeBuilder<'a> { instruction_no_args!(nop, NOP, 0, false); pub fn block(&mut self, ty: BlockType) { - self.inst_imm8(BLOCK, 0, false, ty.as_byte()); + self.inst_block(BLOCK, 0, ty); } pub fn loop_(&mut self, ty: BlockType) { - self.inst_imm8(LOOP, 0, false, ty.as_byte()); + self.inst_block(LOOP, 0, ty); } pub fn if_(&mut self, ty: BlockType) { - self.inst_imm8(IF, 1, false, ty.as_byte()); + self.inst_block(IF, 1, ty); + } + pub fn else_(&mut self) { + // Reuse the 'then' block but clear its value stack + self.current_stack_mut().clear(); + self.inst(ELSE, 0, false); } - instruction_no_args!(else_, ELSE, 0, false); - instruction_no_args!(end, END, 0, false); + pub fn end(&mut self) { + self.inst_base(END, 0, false); + let ended_block = self.vm_block_stack.pop().unwrap(); + if ended_block.has_result { + let result = ended_block.value_stack.last().unwrap(); + self.current_stack_mut().push(*result) + } + + log_instruction!("END \t\t{:?}", &self.vm_block_stack); + } pub fn br(&mut self, levels: u32) { self.inst_imm32(BR, 0, false, levels); } pub fn br_if(&mut self, levels: u32) { + // In dynamic execution, br_if can pop 2 values if condition is true and the target block has a result. + // But our stack model is for *static* analysis and we need it to be correct at the next instruction, + // where the branch was not taken. So we only pop 1 value, the condition. self.inst_imm32(BRIF, 1, false, levels); } #[allow(dead_code)] @@ -520,7 +634,7 @@ impl<'a> CodeBuilder<'a> { n_args: usize, has_return_val: bool, ) { - self.inst(CALL, n_args, has_return_val); + self.inst_base(CALL, n_args, has_return_val); let offset = self.code.len() as u32; self.code.encode_padded_u32(function_index); @@ -533,6 +647,13 @@ impl<'a> CodeBuilder<'a> { offset, symbol_index, }); + + log_instruction!( + "{:10}\t{}\t{:?}", + format!("{:?}", CALL), + function_index, + self.current_stack() + ); } #[allow(dead_code)] @@ -584,26 +705,44 @@ impl<'a> CodeBuilder<'a> { instruction_memargs!(i64_store32, I64STORE32, 2, false); pub fn memory_size(&mut self) { - self.inst_imm8(CURRENTMEMORY, 0, true, 0); + self.inst(CURRENTMEMORY, 0, true); + self.code.push(0); } pub fn memory_grow(&mut self) { - self.inst_imm8(GROWMEMORY, 1, true, 0); + self.inst(GROWMEMORY, 1, true); + self.code.push(0); + } + + fn log_const(&self, opcode: OpCode, x: T) + where + T: std::fmt::Debug + std::fmt::Display, + { + log_instruction!( + "{:10}\t{}\t{:?}", + format!("{:?}", opcode), + x, + self.current_stack() + ); } pub fn i32_const(&mut self, x: i32) { - self.inst(I32CONST, 0, true); + self.inst_base(I32CONST, 0, true); self.code.encode_i32(x); + self.log_const(I32CONST, x); } pub fn i64_const(&mut self, x: i64) { - self.inst(I64CONST, 0, true); + self.inst_base(I64CONST, 0, true); self.code.encode_i64(x); + self.log_const(I64CONST, x); } pub fn f32_const(&mut self, x: f32) { - self.inst(F32CONST, 0, true); + self.inst_base(F32CONST, 0, true); self.code.encode_f32(x); + self.log_const(F32CONST, x); } pub fn f64_const(&mut self, x: f64) { - self.inst(F64CONST, 0, true); + self.inst_base(F64CONST, 0, true); self.code.encode_f64(x); + self.log_const(F64CONST, x); } // TODO: Consider creating unified methods for numerical ops like 'eq' and 'add', diff --git a/compiler/gen_wasm/src/wasm_module/opcodes.rs b/compiler/gen_wasm/src/wasm_module/opcodes.rs index cb8ef70b47..868b77375d 100644 --- a/compiler/gen_wasm/src/wasm_module/opcodes.rs +++ b/compiler/gen_wasm/src/wasm_module/opcodes.rs @@ -1,5 +1,5 @@ #[repr(u8)] -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum OpCode { UNREACHABLE = 0x00, NOP = 0x01, diff --git a/compiler/test_gen/src/gen_primitives.rs b/compiler/test_gen/src/gen_primitives.rs index de4c4f813d..7928fc109e 100644 --- a/compiler/test_gen/src/gen_primitives.rs +++ b/compiler/test_gen/src/gen_primitives.rs @@ -385,7 +385,7 @@ fn gen_basic_def() { } #[test] -#[cfg(any(feature = "gen-llvm"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))] fn gen_multiple_defs() { assert_evals_to!( indoc!( From f121d6f599ccfbd8948f266c8efc58a3e44f6ace Mon Sep 17 00:00:00 2001 From: Brian Carroll Date: Sun, 14 Nov 2021 13:21:56 +0000 Subject: [PATCH 31/44] Fix formatting --- compiler/gen_wasm/src/wasm_module/code_builder.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/compiler/gen_wasm/src/wasm_module/code_builder.rs b/compiler/gen_wasm/src/wasm_module/code_builder.rs index ed2a850576..660f612bdf 100644 --- a/compiler/gen_wasm/src/wasm_module/code_builder.rs +++ b/compiler/gen_wasm/src/wasm_module/code_builder.rs @@ -63,8 +63,11 @@ struct VmBlock<'a> { impl std::fmt::Debug for VmBlock<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let result = if self.has_result { "Result" } else { "NoResult" }; - f.write_fmt(format_args!("{:?} {}", self.opcode, result)) + f.write_fmt(format_args!("{:?} {}", self.opcode, if self.has_result { + "Result" + } else { + "NoResult" + })) } } @@ -308,7 +311,10 @@ impl<'a> CodeBuilder<'a> { self.add_insertion(pushed_at, SETLOCAL, next_local_id.0); } else { if ENABLE_DEBUG_LOG { - println!("{:?} has been popped implicitly. Leaving it on the stack.", symbol); + println!( + "{:?} has been popped implicitly. Leaving it on the stack.", + symbol + ); } self.add_insertion(pushed_at, TEELOCAL, next_local_id.0); } From d34f5050cb65ee5abae0ed9c9623508dbc51e47d Mon Sep 17 00:00:00 2001 From: Brian Carroll Date: Sun, 14 Nov 2021 13:35:25 +0000 Subject: [PATCH 32/44] formatting --- compiler/gen_wasm/src/wasm_module/code_builder.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/compiler/gen_wasm/src/wasm_module/code_builder.rs b/compiler/gen_wasm/src/wasm_module/code_builder.rs index 660f612bdf..5468784926 100644 --- a/compiler/gen_wasm/src/wasm_module/code_builder.rs +++ b/compiler/gen_wasm/src/wasm_module/code_builder.rs @@ -63,11 +63,15 @@ struct VmBlock<'a> { impl std::fmt::Debug for VmBlock<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_fmt(format_args!("{:?} {}", self.opcode, if self.has_result { - "Result" - } else { - "NoResult" - })) + f.write_fmt(format_args!( + "{:?} {}", + self.opcode, + if self.has_result { + "Result" + } else { + "NoResult" + } + )) } } From c10f403c941ba4360853b8a68c78c11f524d1c28 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Wed, 10 Nov 2021 19:28:04 -0800 Subject: [PATCH 33/44] Allow trailing comments in exposes decl --- compiler/load/src/file.rs | 4 +-- compiler/parse/src/header.rs | 9 ++++-- compiler/parse/src/module.rs | 12 ++++---- compiler/parse/src/parser.rs | 1 + compiler/parse/tests/test_parse.rs | 45 ++++++++++++++++++++++++++---- 5 files changed, 55 insertions(+), 16 deletions(-) diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index 0d20be6415..745574b9a7 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -3886,7 +3886,7 @@ fn exposed_from_import<'a>(entry: &ImportsEntry<'a>) -> (QualifiedModuleName<'a> Module(module_name, exposes) => { let mut exposed = Vec::with_capacity(exposes.len()); - for loc_entry in exposes { + for loc_entry in exposes.iter() { exposed.push(ident_from_exposed(&loc_entry.value)); } @@ -3901,7 +3901,7 @@ fn exposed_from_import<'a>(entry: &ImportsEntry<'a>) -> (QualifiedModuleName<'a> Package(package_name, module_name, exposes) => { let mut exposed = Vec::with_capacity(exposes.len()); - for loc_entry in exposes { + for loc_entry in exposes.iter() { exposed.push(ident_from_exposed(&loc_entry.value)); } diff --git a/compiler/parse/src/header.rs b/compiler/parse/src/header.rs index 42c35cd0dc..c5d3696630 100644 --- a/compiler/parse/src/header.rs +++ b/compiler/parse/src/header.rs @@ -177,7 +177,7 @@ pub struct Effects<'a> { pub entries: &'a [Loc>], } -#[derive(Clone, Debug, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq)] pub enum ExposesEntry<'a, T> { /// e.g. `Task` Exposed(T), @@ -199,13 +199,16 @@ impl<'a, T> Spaceable<'a> for ExposesEntry<'a, T> { #[derive(Clone, Debug, PartialEq)] pub enum ImportsEntry<'a> { /// e.g. `Task` or `Task.{ Task, after }` - Module(ModuleName<'a>, Vec<'a, Loc>>), + Module( + ModuleName<'a>, + Collection<'a, Loc>>, + ), /// e.g. `base.Task` or `base.Task.{ after }` or `base.{ Task.{ Task, after } }` Package( &'a str, ModuleName<'a>, - Vec<'a, Loc>>, + Collection<'a, Loc>>, ), // Spaces diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index eec34ecbcb..e33c7daf1d 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -768,7 +768,7 @@ fn imports_entry<'a>() -> impl Parser<'a, ImportsEntry<'a>, EImports> { type Temp<'a> = ( (Option<&'a str>, ModuleName<'a>), - Option>>>, + Option>>>, ); map_with_arena!( @@ -785,19 +785,21 @@ fn imports_entry<'a>() -> impl Parser<'a, ImportsEntry<'a>, EImports> { // e.g. `.{ Task, after}` maybe!(skip_first!( word1(b'.', EImports::ExposingDot), - collection_e!( + collection_trailing_sep_e!( word1(b'{', EImports::SetStart), exposes_entry(EImports::Identifier), word1(b',', EImports::SetEnd), word1(b'}', EImports::SetEnd), min_indent, + EImports::Open, EImports::Space, - EImports::IndentSetEnd + EImports::IndentSetEnd, + ExposesEntry::SpaceBefore ) )) ), - |arena, ((opt_shortname, module_name), opt_values): Temp<'a>| { - let exposed_values = opt_values.unwrap_or_else(|| Vec::new_in(arena)); + |_arena, ((opt_shortname, module_name), opt_values): Temp<'a>| { + let exposed_values = opt_values.unwrap_or_else(|| Collection::empty()); match opt_shortname { Some(shortname) => ImportsEntry::Package(shortname, module_name, exposed_values), diff --git a/compiler/parse/src/parser.rs b/compiler/parse/src/parser.rs index 07b9800359..ece23b6c2b 100644 --- a/compiler/parse/src/parser.rs +++ b/compiler/parse/src/parser.rs @@ -315,6 +315,7 @@ pub enum EEffects<'a> { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum EImports { + Open(Row, Col), Imports(Row, Col), IndentImports(Row, Col), IndentListStart(Row, Col), diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index c49820045b..041123c9e7 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -3166,7 +3166,7 @@ mod test_parse { let loc_pkg_entry = Located::new(1, 1, 15, 33, pkg_entry); let arena = Bump::new(); let packages = Collection::with_items(arena.alloc([loc_pkg_entry])); - let import = ImportsEntry::Package("foo", ModuleName::new("Bar.Baz"), Vec::new_in(&arena)); + let import = ImportsEntry::Package("foo", ModuleName::new("Bar.Baz"), Collection::empty()); let loc_import = Located::new(2, 2, 14, 25, import); let imports = bumpalo::vec![in &arena; loc_import]; let provide_entry = Located::new(3, 3, 15, 24, Exposed("quicksort")); @@ -3222,10 +3222,39 @@ mod test_parse { let loc_pkg_entry = Located::new(1, 1, 15, 33, pkg_entry); let arena = Bump::new(); let packages = Collection::with_items(arena.alloc([loc_pkg_entry])); - let import = ImportsEntry::Package("foo", ModuleName::new("Bar.Baz"), Vec::new_in(&arena)); - let loc_import = Located::new(2, 2, 14, 25, import); + let import = ImportsEntry::Package( + "foo", + ModuleName::new("Bar"), + Collection::with_items_and_comments( + &arena, + arena.alloc([ + Located::new( + 3, + 3, + 8, + 11, + ExposesEntry::SpaceBefore( + arena.alloc(ExposesEntry::Exposed("Baz")), + arena.alloc([Newline]), + ), + ), + Located::new( + 4, + 4, + 8, + 17, + ExposesEntry::SpaceBefore( + arena.alloc(ExposesEntry::Exposed("FourtyTwo")), + arena.alloc([Newline]), + ), + ), + ]), + arena.alloc([Newline, LineComment(" I'm a happy comment")]), + ), + ); + let loc_import = Located::new(2, 6, 14, 5, import); let imports = bumpalo::vec![in &arena; loc_import]; - let provide_entry = Located::new(3, 3, 15, 24, Exposed("quicksort")); + let provide_entry = Located::new(7, 7, 15, 24, Exposed("quicksort")); let provides = bumpalo::vec![in &arena; provide_entry]; let module_name = StrLiteral::PlainLine("quicksort"); @@ -3235,7 +3264,7 @@ mod test_parse { packages, imports, provides, - to: Located::new(3, 3, 30, 34, To::ExistingPackage("base")), + to: Located::new(7, 7, 30, 34, To::ExistingPackage("base")), after_app_keyword: &[], before_packages: newlines, after_packages: &[], @@ -3253,7 +3282,11 @@ mod test_parse { r#" app "quicksort" packages { base: "./platform", } - imports [ foo.Bar.Baz ] + imports [ foo.Bar.{ + Baz, + FourtyTwo, + # I'm a happy comment + } ] provides [ quicksort ] to base "# ); From 23c75d269988890f911deae2e3eb58fde7763094 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Wed, 10 Nov 2021 19:39:02 -0800 Subject: [PATCH 34/44] Allow trailing comments in imports decl --- compiler/fmt/src/module.rs | 8 ++++---- compiler/load/src/file.rs | 6 +++--- compiler/parse/src/header.rs | 10 +++++----- compiler/parse/src/module.rs | 12 +++++++----- compiler/parse/tests/test_parse.rs | 16 ++++++++-------- 5 files changed, 27 insertions(+), 25 deletions(-) diff --git a/compiler/fmt/src/module.rs b/compiler/fmt/src/module.rs index 6ceca3d28c..e0ba55a451 100644 --- a/compiler/fmt/src/module.rs +++ b/compiler/fmt/src/module.rs @@ -1,6 +1,6 @@ use crate::spaces::{fmt_spaces, INDENT}; use bumpalo::collections::{String, Vec}; -use roc_parse::ast::Module; +use roc_parse::ast::{Collection, Module}; use roc_parse::header::{AppHeader, ExposesEntry, ImportsEntry, InterfaceHeader, PlatformHeader}; use roc_region::all::Located; @@ -64,7 +64,7 @@ pub fn fmt_interface_header<'a>(buf: &mut String<'a>, header: &'a InterfaceHeade fmt_spaces(buf, header.after_imports.iter(), indent); } - fmt_imports(buf, &header.imports, indent); + fmt_imports(buf, header.imports, indent); } pub fn fmt_app_header<'a>(buf: &mut String<'a>, header: &'a AppHeader<'a>) { @@ -76,7 +76,7 @@ pub fn fmt_app_header<'a>(buf: &mut String<'a>, header: &'a AppHeader<'a>) { buf.push_str("imports"); fmt_spaces(buf, header.before_imports.iter(), indent); - fmt_imports(buf, &header.imports, indent); + fmt_imports(buf, header.imports, indent); fmt_spaces(buf, header.after_imports.iter(), indent); } @@ -86,7 +86,7 @@ pub fn fmt_platform_header<'a>(_buf: &mut String<'a>, _header: &'a PlatformHeade fn fmt_imports<'a>( buf: &mut String<'a>, - loc_entries: &'a Vec<'a, Located>>, + loc_entries: Collection<'a, Located>>, indent: u16, ) { buf.push('['); diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index 745574b9a7..2a4bd23738 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -2609,7 +2609,7 @@ fn parse_header<'a>( header_src, packages: &[], exposes: header.exposes.into_bump_slice(), - imports: header.imports.into_bump_slice(), + imports: header.imports.items, to_platform: None, }; @@ -2643,7 +2643,7 @@ fn parse_header<'a>( header_src, packages, exposes: header.provides.into_bump_slice(), - imports: header.imports.into_bump_slice(), + imports: header.imports.items, to_platform: Some(header.to.value.clone()), }; @@ -3421,7 +3421,7 @@ fn fabricate_pkg_config_module<'a>( packages: &[], provides, requires: arena.alloc([header.requires.signature.clone()]), - imports: header.imports.clone().into_bump_slice(), + imports: header.imports.items, }; send_header_two( diff --git a/compiler/parse/src/header.rs b/compiler/parse/src/header.rs index c5d3696630..d0c76d22c5 100644 --- a/compiler/parse/src/header.rs +++ b/compiler/parse/src/header.rs @@ -38,7 +38,7 @@ pub enum PackageOrPath<'a> { Path(StrLiteral<'a>), } -#[derive(Clone, PartialEq, Eq, Debug, Hash)] +#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] pub struct ModuleName<'a>(&'a str); impl<'a> From> for &'a str { @@ -61,7 +61,7 @@ impl<'a> ModuleName<'a> { pub struct InterfaceHeader<'a> { pub name: Loc>, pub exposes: Vec<'a, Loc>>, - pub imports: Vec<'a, Loc>>, + pub imports: Collection<'a, Loc>>, // Potential comments and newlines - these will typically all be empty. pub before_header: &'a [CommentOrNewline<'a>], @@ -82,7 +82,7 @@ pub enum To<'a> { pub struct AppHeader<'a> { pub name: Loc>, pub packages: Collection<'a, Loc>>, - pub imports: Vec<'a, Loc>>, + pub imports: Collection<'a, Loc>>, pub provides: Vec<'a, Loc>>, pub to: Loc>, @@ -147,7 +147,7 @@ pub struct PlatformHeader<'a> { pub requires: PlatformRequires<'a>, pub exposes: Vec<'a, Loc>>>, pub packages: Collection<'a, Loc>>, - pub imports: Vec<'a, Loc>>, + pub imports: Collection<'a, Loc>>, pub provides: Vec<'a, Loc>>, pub effects: Effects<'a>, @@ -196,7 +196,7 @@ impl<'a, T> Spaceable<'a> for ExposesEntry<'a, T> { } } -#[derive(Clone, Debug, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq)] pub enum ImportsEntry<'a> { /// e.g. `Task` or `Task.{ Task, after }` Module( diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index e33c7daf1d..56f22610d9 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -220,11 +220,11 @@ fn app_header<'a>() -> impl Parser<'a, AppHeader<'a>, EHeader<'a>> { #[allow(clippy::type_complexity)] let opt_imports: Option<( (&'a [CommentOrNewline<'a>], &'a [CommentOrNewline<'a>]), - Vec<'a, Located>>, + Collection<'a, Located>>, )> = opt_imports; let ((before_imports, after_imports), imports) = - opt_imports.unwrap_or_else(|| ((&[] as _, &[] as _), Vec::new_in(arena))); + opt_imports.unwrap_or_else(|| ((&[] as _, &[] as _), Collection::empty())); let provides: ProvidesTo<'a> = provides; // rustc must be told the type here let header = AppHeader { @@ -631,7 +631,7 @@ fn imports<'a>() -> impl Parser< 'a, ( (&'a [CommentOrNewline<'a>], &'a [CommentOrNewline<'a>]), - Vec<'a, Located>>, + Collection<'a, Located>>, ), EImports, > { @@ -646,14 +646,16 @@ fn imports<'a>() -> impl Parser< EImports::IndentImports, EImports::IndentListStart ), - collection_e!( + collection_trailing_sep_e!( word1(b'[', EImports::ListStart), loc!(imports_entry()), word1(b',', EImports::ListEnd), word1(b']', EImports::ListEnd), min_indent, + EImports::Open, EImports::Space, - EImports::IndentListEnd + EImports::IndentListEnd, + ImportsEntry::SpaceBefore ) ) } diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index 041123c9e7..5782b1d239 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -3077,7 +3077,7 @@ mod test_parse { fn empty_app_header() { let arena = Bump::new(); let packages = Collection::empty(); - let imports = Vec::new_in(&arena); + let imports = Collection::empty(); let provides = Vec::new_in(&arena); let module_name = StrLiteral::PlainLine("test-app"); let header = AppHeader { @@ -3117,7 +3117,7 @@ mod test_parse { let arena = Bump::new(); let packages = Collection::empty(); - let imports = Vec::new_in(&arena); + let imports = Collection::empty(); let provides = Vec::new_in(&arena); let module_name = StrLiteral::PlainLine("test-app"); let header = AppHeader { @@ -3168,7 +3168,7 @@ mod test_parse { let packages = Collection::with_items(arena.alloc([loc_pkg_entry])); let import = ImportsEntry::Package("foo", ModuleName::new("Bar.Baz"), Collection::empty()); let loc_import = Located::new(2, 2, 14, 25, import); - let imports = bumpalo::vec![in &arena; loc_import]; + let imports = Collection::with_items(arena.alloc([loc_import])); let provide_entry = Located::new(3, 3, 15, 24, Exposed("quicksort")); let provides = bumpalo::vec![in &arena; provide_entry]; let module_name = StrLiteral::PlainLine("quicksort"); @@ -3253,7 +3253,7 @@ mod test_parse { ), ); let loc_import = Located::new(2, 6, 14, 5, import); - let imports = bumpalo::vec![in &arena; loc_import]; + let imports = Collection::with_items(arena.alloc([loc_import])); let provide_entry = Located::new(7, 7, 15, 24, Exposed("quicksort")); let provides = bumpalo::vec![in &arena; provide_entry]; let module_name = StrLiteral::PlainLine("quicksort"); @@ -3342,7 +3342,7 @@ mod test_parse { requires, exposes: Vec::new_in(&arena), packages: Collection::empty(), - imports: Vec::new_in(&arena), + imports: Collection::empty(), provides: Vec::new_in(&arena), effects, after_platform_keyword: &[], @@ -3385,7 +3385,7 @@ mod test_parse { let loc_pkg_entry = Located::new(3, 3, 15, 27, pkg_entry); let arena = Bump::new(); let packages = Collection::with_items(arena.alloc([loc_pkg_entry])); - let imports = Vec::new_in(&arena); + let imports = Collection::empty(); let provide_entry = Located::new(5, 5, 15, 26, Exposed("mainForHost")); let provides = bumpalo::vec![in &arena; provide_entry]; let effects = Effects { @@ -3466,7 +3466,7 @@ mod test_parse { fn empty_interface_header() { let arena = Bump::new(); let exposes = Vec::new_in(&arena); - let imports = Vec::new_in(&arena); + let imports = Collection::empty(); let module_name = ModuleName::new("Foo"); let header = InterfaceHeader { before_header: &[], @@ -3498,7 +3498,7 @@ mod test_parse { fn nested_module() { let arena = Bump::new(); let exposes = Vec::new_in(&arena); - let imports = Vec::new_in(&arena); + let imports = Collection::empty(); let module_name = ModuleName::new("Foo.Bar.Baz"); let header = InterfaceHeader { before_header: &[], From d67b6c50b106fe74da7add5f567c8743ee42a440 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Wed, 10 Nov 2021 20:20:55 -0800 Subject: [PATCH 35/44] Allow trailing comments in provides decl --- compiler/parse/src/module.rs | 15 ++++++++++----- compiler/parse/src/parser.rs | 1 + compiler/parse/tests/test_parse.rs | 4 ++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index 56f22610d9..004a17a6f8 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -270,7 +270,7 @@ fn platform_header<'a>() -> impl Parser<'a, PlatformHeader<'a>, EHeader<'a>> { let (_, ((before_imports, after_imports), imports), state) = specialize(EHeader::Imports, imports()).parse(arena, state)?; - let (_, ((before_provides, after_provides), provides), state) = + let (_, ((before_provides, after_provides), (provides, provides_final_comments)), state) = specialize(EHeader::Provides, provides_without_to()).parse(arena, state)?; let (_, effects, state) = specialize(EHeader::Effects, effects()).parse(arena, state)?; @@ -342,7 +342,7 @@ fn provides_to<'a>() -> impl Parser<'a, ProvidesTo<'a>, EProvides<'a>> { ) ), |( - ((before_provides_keyword, after_provides_keyword), entries), + ((before_provides_keyword, after_provides_keyword), (entries, final_comments)), ((before_to_keyword, after_to_keyword), to), )| { ProvidesTo { @@ -362,7 +362,10 @@ fn provides_without_to<'a>() -> impl Parser< 'a, ( (&'a [CommentOrNewline<'a>], &'a [CommentOrNewline<'a>]), - Vec<'a, Located>>, + ( + Vec<'a, Located>>, + &'a [CommentOrNewline<'a>], + ), ), EProvides<'a>, > { @@ -376,14 +379,16 @@ fn provides_without_to<'a>() -> impl Parser< EProvides::IndentProvides, EProvides::IndentListStart ), - collection_e!( + collection_trailing_sep_e!( word1(b'[', EProvides::ListStart), exposes_entry(EProvides::Identifier), word1(b',', EProvides::ListEnd), word1(b']', EProvides::ListEnd), min_indent, + EProvides::Open, EProvides::Space, - EProvides::IndentListEnd + EProvides::IndentListEnd, + ExposesEntry::SpaceBefore ) ) } diff --git a/compiler/parse/src/parser.rs b/compiler/parse/src/parser.rs index ece23b6c2b..4b0270e74a 100644 --- a/compiler/parse/src/parser.rs +++ b/compiler/parse/src/parser.rs @@ -214,6 +214,7 @@ pub enum EHeader<'a> { #[derive(Debug, Clone, PartialEq, Eq)] pub enum EProvides<'a> { Provides(Row, Col), + Open(Row, Col), To(Row, Col), IndentProvides(Row, Col), IndentTo(Row, Col), diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index 5782b1d239..4d3fd5e2ba 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -3264,7 +3264,7 @@ mod test_parse { packages, imports, provides, - to: Located::new(7, 7, 30, 34, To::ExistingPackage("base")), + to: Located::new(7, 7, 31, 35, To::ExistingPackage("base")), after_app_keyword: &[], before_packages: newlines, after_packages: &[], @@ -3287,7 +3287,7 @@ mod test_parse { FourtyTwo, # I'm a happy comment } ] - provides [ quicksort ] to base + provides [ quicksort, ] to base "# ); From df89fe7dd686960cf76506fc57a5527c72edc27b Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 10:58:45 -0800 Subject: [PATCH 36/44] Make provides a Collection --- compiler/load/src/file.rs | 4 ++-- compiler/parse/src/header.rs | 4 ++-- compiler/parse/src/module.rs | 11 ++++------- compiler/parse/tests/test_parse.rs | 12 ++++++------ 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index 2a4bd23738..11c30c2dc7 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -2642,7 +2642,7 @@ fn parse_header<'a>( opt_shorthand, header_src, packages, - exposes: header.provides.into_bump_slice(), + exposes: header.provides.items, imports: header.imports.items, to_platform: Some(header.to.value.clone()), }; @@ -3410,7 +3410,7 @@ fn fabricate_pkg_config_module<'a>( module_timing: ModuleTiming, ) -> (ModuleId, Msg<'a>) { let provides: &'a [Located>] = - header.provides.clone().into_bump_slice(); + header.provides.items; let info = PlatformHeaderInfo { filename, diff --git a/compiler/parse/src/header.rs b/compiler/parse/src/header.rs index d0c76d22c5..37475ebce1 100644 --- a/compiler/parse/src/header.rs +++ b/compiler/parse/src/header.rs @@ -83,7 +83,7 @@ pub struct AppHeader<'a> { pub name: Loc>, pub packages: Collection<'a, Loc>>, pub imports: Collection<'a, Loc>>, - pub provides: Vec<'a, Loc>>, + pub provides: Collection<'a, Loc>>, pub to: Loc>, // Potential comments and newlines - these will typically all be empty. @@ -148,7 +148,7 @@ pub struct PlatformHeader<'a> { pub exposes: Vec<'a, Loc>>>, pub packages: Collection<'a, Loc>>, pub imports: Collection<'a, Loc>>, - pub provides: Vec<'a, Loc>>, + pub provides: Collection<'a, Loc>>, pub effects: Effects<'a>, // Potential comments and newlines - these will typically all be empty. diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index 004a17a6f8..4a31995e91 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -270,7 +270,7 @@ fn platform_header<'a>() -> impl Parser<'a, PlatformHeader<'a>, EHeader<'a>> { let (_, ((before_imports, after_imports), imports), state) = specialize(EHeader::Imports, imports()).parse(arena, state)?; - let (_, ((before_provides, after_provides), (provides, provides_final_comments)), state) = + let (_, ((before_provides, after_provides), provides), state) = specialize(EHeader::Provides, provides_without_to()).parse(arena, state)?; let (_, effects, state) = specialize(EHeader::Effects, effects()).parse(arena, state)?; @@ -303,7 +303,7 @@ fn platform_header<'a>() -> impl Parser<'a, PlatformHeader<'a>, EHeader<'a>> { #[derive(Debug)] struct ProvidesTo<'a> { - entries: Vec<'a, Located>>, + entries: Collection<'a, Located>>, to: Located>, before_provides_keyword: &'a [CommentOrNewline<'a>], @@ -342,7 +342,7 @@ fn provides_to<'a>() -> impl Parser<'a, ProvidesTo<'a>, EProvides<'a>> { ) ), |( - ((before_provides_keyword, after_provides_keyword), (entries, final_comments)), + ((before_provides_keyword, after_provides_keyword), entries), ((before_to_keyword, after_to_keyword), to), )| { ProvidesTo { @@ -362,10 +362,7 @@ fn provides_without_to<'a>() -> impl Parser< 'a, ( (&'a [CommentOrNewline<'a>], &'a [CommentOrNewline<'a>]), - ( - Vec<'a, Located>>, - &'a [CommentOrNewline<'a>], - ), + Collection<'a, Located>>, ), EProvides<'a>, > { diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index 4d3fd5e2ba..b3b3779bd4 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -3078,7 +3078,7 @@ mod test_parse { let arena = Bump::new(); let packages = Collection::empty(); let imports = Collection::empty(); - let provides = Vec::new_in(&arena); + let provides = Collection::empty(); let module_name = StrLiteral::PlainLine("test-app"); let header = AppHeader { name: Located::new(0, 0, 4, 14, module_name), @@ -3118,7 +3118,7 @@ mod test_parse { let arena = Bump::new(); let packages = Collection::empty(); let imports = Collection::empty(); - let provides = Vec::new_in(&arena); + let provides = Collection::empty(); let module_name = StrLiteral::PlainLine("test-app"); let header = AppHeader { before_header: &[], @@ -3170,7 +3170,7 @@ mod test_parse { let loc_import = Located::new(2, 2, 14, 25, import); let imports = Collection::with_items(arena.alloc([loc_import])); let provide_entry = Located::new(3, 3, 15, 24, Exposed("quicksort")); - let provides = bumpalo::vec![in &arena; provide_entry]; + let provides = Collection::with_items(arena.alloc([provide_entry])); let module_name = StrLiteral::PlainLine("quicksort"); let header = AppHeader { @@ -3255,7 +3255,7 @@ mod test_parse { let loc_import = Located::new(2, 6, 14, 5, import); let imports = Collection::with_items(arena.alloc([loc_import])); let provide_entry = Located::new(7, 7, 15, 24, Exposed("quicksort")); - let provides = bumpalo::vec![in &arena; provide_entry]; + let provides = Collection::with_items(arena.alloc([provide_entry])); let module_name = StrLiteral::PlainLine("quicksort"); let header = AppHeader { @@ -3343,7 +3343,7 @@ mod test_parse { exposes: Vec::new_in(&arena), packages: Collection::empty(), imports: Collection::empty(), - provides: Vec::new_in(&arena), + provides: Collection::empty(), effects, after_platform_keyword: &[], before_requires: &[], @@ -3387,7 +3387,7 @@ mod test_parse { let packages = Collection::with_items(arena.alloc([loc_pkg_entry])); let imports = Collection::empty(); let provide_entry = Located::new(5, 5, 15, 26, Exposed("mainForHost")); - let provides = bumpalo::vec![in &arena; provide_entry]; + let provides = Collection::with_items(arena.alloc([provide_entry])); let effects = Effects { effect_type_name: "Effect", effect_shortname: "fx", From 71cc8d4c4b7b8fb3e5fe28833e95826c179c783d Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 11:09:52 -0800 Subject: [PATCH 37/44] Convert requires_rigids to collection_trailing_sep_e --- compiler/load/src/file.rs | 3 +-- compiler/parse/src/header.rs | 4 ++-- compiler/parse/src/module.rs | 8 +++++--- compiler/parse/src/parser.rs | 1 + compiler/parse/tests/test_parse.rs | 10 ++++++++-- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index 11c30c2dc7..eb77687428 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -3409,8 +3409,7 @@ fn fabricate_pkg_config_module<'a>( header_src: &'a str, module_timing: ModuleTiming, ) -> (ModuleId, Msg<'a>) { - let provides: &'a [Located>] = - header.provides.items; + let provides: &'a [Located>] = header.provides.items; let info = PlatformHeaderInfo { filename, diff --git a/compiler/parse/src/header.rs b/compiler/parse/src/header.rs index 37475ebce1..f7ec7139b9 100644 --- a/compiler/parse/src/header.rs +++ b/compiler/parse/src/header.rs @@ -117,7 +117,7 @@ pub struct PackageHeader<'a> { pub after_imports: &'a [CommentOrNewline<'a>], } -#[derive(Clone, Debug, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq)] pub enum PlatformRigid<'a> { Entry { rigid: &'a str, alias: &'a str }, @@ -137,7 +137,7 @@ impl<'a> Spaceable<'a> for PlatformRigid<'a> { #[derive(Clone, Debug, PartialEq)] pub struct PlatformRequires<'a> { - pub rigids: Vec<'a, Loc>>, + pub rigids: Collection<'a, Loc>>, pub signature: Loc>, } diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index 4a31995e91..e6a8d9a6e3 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -444,15 +444,17 @@ fn platform_requires<'a>() -> impl Parser<'a, PlatformRequires<'a>, ERequires<'a #[inline(always)] fn requires_rigids<'a>( min_indent: u16, -) -> impl Parser<'a, Vec<'a, Located>>, ERequires<'a>> { - collection_e!( +) -> impl Parser<'a, Collection<'a, Located>>, ERequires<'a>> { + collection_trailing_sep_e!( word1(b'{', ERequires::ListStart), specialize(|_, r, c| ERequires::Rigid(r, c), loc!(requires_rigid())), word1(b',', ERequires::ListEnd), word1(b'}', ERequires::ListEnd), min_indent, + ERequires::Open, ERequires::Space, - ERequires::IndentListEnd + ERequires::IndentListEnd, + PlatformRigid::SpaceBefore ) } diff --git a/compiler/parse/src/parser.rs b/compiler/parse/src/parser.rs index 4b0270e74a..1c176893d5 100644 --- a/compiler/parse/src/parser.rs +++ b/compiler/parse/src/parser.rs @@ -243,6 +243,7 @@ pub enum EExposes { #[derive(Debug, Clone, PartialEq, Eq)] pub enum ERequires<'a> { Requires(Row, Col), + Open(Row, Col), IndentRequires(Row, Col), IndentListStart(Row, Col), IndentListEnd(Row, Col), diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index b3b3779bd4..5a194088f5 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -3318,7 +3318,7 @@ mod test_parse { let region2 = Region::new(0, 0, 45, 47); PlatformRequires { - rigids: Vec::new_in(&arena), + rigids: Collection::empty(), signature: Located::at( region1, TypedIdent::Entry { @@ -3403,7 +3403,13 @@ mod test_parse { let region3 = Region::new(1, 1, 14, 26); PlatformRequires { - rigids: bumpalo::vec![ in &arena; Located::at(region3, PlatformRigid::Entry { alias: "Model", rigid: "model" }) ], + rigids: Collection::with_items(arena.alloc([Located::at( + region3, + PlatformRigid::Entry { + alias: "Model", + rigid: "model", + }, + )])), signature: Located::at( region1, TypedIdent::Entry { From c4e70ca7aa6b268c014439a414f27449f5cb300d Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 11:12:17 -0800 Subject: [PATCH 38/44] Convert exposes_values to ccollection_trailing_sep_e --- compiler/fmt/src/module.rs | 4 ++-- compiler/load/src/file.rs | 2 +- compiler/parse/src/header.rs | 2 +- compiler/parse/src/module.rs | 8 +++++--- compiler/parse/src/parser.rs | 1 + compiler/parse/tests/test_parse.rs | 4 ++-- 6 files changed, 12 insertions(+), 9 deletions(-) diff --git a/compiler/fmt/src/module.rs b/compiler/fmt/src/module.rs index e0ba55a451..17522326c2 100644 --- a/compiler/fmt/src/module.rs +++ b/compiler/fmt/src/module.rs @@ -1,5 +1,5 @@ use crate::spaces::{fmt_spaces, INDENT}; -use bumpalo::collections::{String, Vec}; +use bumpalo::collections::String; use roc_parse::ast::{Collection, Module}; use roc_parse::header::{AppHeader, ExposesEntry, ImportsEntry, InterfaceHeader, PlatformHeader}; use roc_region::all::Located; @@ -112,7 +112,7 @@ fn fmt_imports<'a>( fn fmt_exposes<'a>( buf: &mut String<'a>, - loc_entries: &'a Vec<'a, Located>>, + loc_entries: &'a Collection<'a, Located>>, indent: u16, ) { buf.push('['); diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index eb77687428..89e610ef0b 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -2608,7 +2608,7 @@ fn parse_header<'a>( opt_shorthand, header_src, packages: &[], - exposes: header.exposes.into_bump_slice(), + exposes: header.exposes.items, imports: header.imports.items, to_platform: None, }; diff --git a/compiler/parse/src/header.rs b/compiler/parse/src/header.rs index f7ec7139b9..f40c0d6780 100644 --- a/compiler/parse/src/header.rs +++ b/compiler/parse/src/header.rs @@ -60,7 +60,7 @@ impl<'a> ModuleName<'a> { #[derive(Clone, Debug, PartialEq)] pub struct InterfaceHeader<'a> { pub name: Loc>, - pub exposes: Vec<'a, Loc>>, + pub exposes: Collection<'a, Loc>>, pub imports: Collection<'a, Loc>>, // Potential comments and newlines - these will typically all be empty. diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index e6a8d9a6e3..00583fbb5a 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -491,7 +491,7 @@ fn exposes_values<'a>() -> impl Parser< 'a, ( (&'a [CommentOrNewline<'a>], &'a [CommentOrNewline<'a>]), - Vec<'a, Located>>, + Collection<'a, Located>>, ), EExposes, > { @@ -506,14 +506,16 @@ fn exposes_values<'a>() -> impl Parser< EExposes::IndentExposes, EExposes::IndentListStart ), - collection_e!( + collection_trailing_sep_e!( word1(b'[', EExposes::ListStart), exposes_entry(EExposes::Identifier), word1(b',', EExposes::ListEnd), word1(b']', EExposes::ListEnd), min_indent, + EExposes::Open, EExposes::Space, - EExposes::IndentListEnd + EExposes::IndentListEnd, + ExposesEntry::SpaceBefore ) ) } diff --git a/compiler/parse/src/parser.rs b/compiler/parse/src/parser.rs index 1c176893d5..73a32128f7 100644 --- a/compiler/parse/src/parser.rs +++ b/compiler/parse/src/parser.rs @@ -231,6 +231,7 @@ pub enum EProvides<'a> { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum EExposes { Exposes(Row, Col), + Open(Row, Col), IndentExposes(Row, Col), IndentListStart(Row, Col), IndentListEnd(Row, Col), diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index 5a194088f5..db54abf04f 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -3471,7 +3471,7 @@ mod test_parse { #[test] fn empty_interface_header() { let arena = Bump::new(); - let exposes = Vec::new_in(&arena); + let exposes = Collection::empty(); let imports = Collection::empty(); let module_name = ModuleName::new("Foo"); let header = InterfaceHeader { @@ -3503,7 +3503,7 @@ mod test_parse { #[test] fn nested_module() { let arena = Bump::new(); - let exposes = Vec::new_in(&arena); + let exposes = Collection::empty(); let imports = Collection::empty(); let module_name = ModuleName::new("Foo.Bar.Baz"); let header = InterfaceHeader { From 8c8bc910fd1c32d8f986298974cee4cd9dc612a9 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 11:13:54 -0800 Subject: [PATCH 39/44] Convert exposes_modules to collection_trailing_sep_e --- compiler/load/src/file.rs | 2 +- compiler/parse/src/header.rs | 2 +- compiler/parse/src/module.rs | 8 +++++--- compiler/parse/tests/test_parse.rs | 4 ++-- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index 89e610ef0b..c733460053 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -3466,7 +3466,7 @@ fn fabricate_effects_module<'a>( { let mut module_ids = (*module_ids).lock(); - for exposed in header.exposes { + for exposed in header.exposes.iter() { if let ExposesEntry::Exposed(module_name) = exposed.value { module_ids.get_or_insert(&PQModuleName::Qualified( shorthand, diff --git a/compiler/parse/src/header.rs b/compiler/parse/src/header.rs index f40c0d6780..20e680ca2f 100644 --- a/compiler/parse/src/header.rs +++ b/compiler/parse/src/header.rs @@ -145,7 +145,7 @@ pub struct PlatformRequires<'a> { pub struct PlatformHeader<'a> { pub name: Loc>, pub requires: PlatformRequires<'a>, - pub exposes: Vec<'a, Loc>>>, + pub exposes: Collection<'a, Loc>>>, pub packages: Collection<'a, Loc>>, pub imports: Collection<'a, Loc>>, pub provides: Collection<'a, Loc>>, diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index 00583fbb5a..f6a8a60148 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -545,7 +545,7 @@ fn exposes_modules<'a>() -> impl Parser< 'a, ( (&'a [CommentOrNewline<'a>], &'a [CommentOrNewline<'a>]), - Vec<'a, Located>>>, + Collection<'a, Located>>>, ), EExposes, > { @@ -560,14 +560,16 @@ fn exposes_modules<'a>() -> impl Parser< EExposes::IndentExposes, EExposes::IndentListStart ), - collection_e!( + collection_trailing_sep_e!( word1(b'[', EExposes::ListStart), exposes_module(EExposes::Identifier), word1(b',', EExposes::ListEnd), word1(b']', EExposes::ListEnd), min_indent, + EExposes::Open, EExposes::Space, - EExposes::IndentListEnd + EExposes::IndentListEnd, + ExposesEntry::SpaceBefore ) ) } diff --git a/compiler/parse/tests/test_parse.rs b/compiler/parse/tests/test_parse.rs index db54abf04f..5032dcc010 100644 --- a/compiler/parse/tests/test_parse.rs +++ b/compiler/parse/tests/test_parse.rs @@ -3340,7 +3340,7 @@ mod test_parse { before_header: &[], name: Located::new(0, 0, 9, 23, pkg_name), requires, - exposes: Vec::new_in(&arena), + exposes: Collection::empty(), packages: Collection::empty(), imports: Collection::empty(), provides: Collection::empty(), @@ -3431,7 +3431,7 @@ mod test_parse { before_header: &[], name: Located::new(0, 0, 9, 19, pkg_name), requires, - exposes: Vec::new_in(&arena), + exposes: Collection::empty(), packages, imports, provides, From 6c82b1789a17ac75cf0bf3c8f457fd0395df1110 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 11:15:56 -0800 Subject: [PATCH 40/44] Convert effects to collection_trailing_sep_e --- compiler/parse/src/header.rs | 2 +- compiler/parse/src/module.rs | 8 +++++--- compiler/parse/src/parser.rs | 1 + 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/compiler/parse/src/header.rs b/compiler/parse/src/header.rs index 20e680ca2f..b1fbcc23eb 100644 --- a/compiler/parse/src/header.rs +++ b/compiler/parse/src/header.rs @@ -227,7 +227,7 @@ impl<'a> ExposesEntry<'a, &'a str> { } } -#[derive(Clone, Debug, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq)] pub enum TypedIdent<'a> { /// e.g. /// diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index f6a8a60148..7a194fe073 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -697,14 +697,16 @@ fn effects<'a>() -> impl Parser<'a, Effects<'a>, EEffects<'a>> { space0_e(min_indent, EEffects::Space, EEffects::IndentListStart) ) .parse(arena, state)?; - let (_, entries, state) = collection_e!( + let (_, entries, state) = collection_trailing_sep_e!( word1(b'{', EEffects::ListStart), specialize(EEffects::TypedIdent, loc!(typed_ident())), word1(b',', EEffects::ListEnd), word1(b'}', EEffects::ListEnd), min_indent, + EEffects::Open, EEffects::Space, - EEffects::IndentListEnd + EEffects::IndentListEnd, + TypedIdent::SpaceBefore ) .parse(arena, state)?; @@ -716,7 +718,7 @@ fn effects<'a>() -> impl Parser<'a, Effects<'a>, EEffects<'a>> { spaces_after_type_name, effect_shortname: type_shortname, effect_type_name: type_name, - entries: entries.into_bump_slice(), + entries: entries.items, }, state, )) diff --git a/compiler/parse/src/parser.rs b/compiler/parse/src/parser.rs index 73a32128f7..9baa462522 100644 --- a/compiler/parse/src/parser.rs +++ b/compiler/parse/src/parser.rs @@ -305,6 +305,7 @@ pub enum EPackageEntry<'a> { pub enum EEffects<'a> { Space(BadInputError, Row, Col), Effects(Row, Col), + Open(Row, Col), IndentEffects(Row, Col), ListStart(Row, Col), ListEnd(Row, Col), From fb960d98b81b20488f8b8077aa04ebea4273623e Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Fri, 12 Nov 2021 11:16:17 -0800 Subject: [PATCH 41/44] Remove unused macros --- compiler/parse/src/parser.rs | 77 ------------------------------------ 1 file changed, 77 deletions(-) diff --git a/compiler/parse/src/parser.rs b/compiler/parse/src/parser.rs index 9baa462522..76e1480387 100644 --- a/compiler/parse/src/parser.rs +++ b/compiler/parse/src/parser.rs @@ -1188,83 +1188,6 @@ macro_rules! collection { }; } -#[macro_export] -macro_rules! collection_e { - ($opening_brace:expr, $elem:expr, $delimiter:expr, $closing_brace:expr, $min_indent:expr, $space_problem:expr, $indent_problem:expr) => { - skip_first!( - $opening_brace, - skip_first!( - // We specifically allow space characters inside here, so that - // `[ ]` can be successfully parsed as an empty list, and then - // changed by the formatter back into `[]`. - // - // We don't allow newlines or comments in the middle of empty - // roc_collections because those are normally stored in an Expr, - // and there's no Expr in which to store them in an empty collection! - // - // We could change the AST to add extra storage specifically to - // support empty literals containing newlines or comments, but this - // does not seem worth even the tiniest regression in compiler performance. - zero_or_more!($crate::parser::word1(b' ', |row, col| $space_problem( - crate::parser::BadInputError::LineTooLong, - row, - col - ))), - skip_second!( - $crate::parser::sep_by0( - $delimiter, - $crate::blankspace::space0_around_ee( - $elem, - $min_indent, - $space_problem, - $indent_problem, - $indent_problem - ) - ), - $closing_brace - ) - ) - ) - }; -} - -/// Parse zero or more elements between two braces (e.g. square braces). -/// Elements can be optionally surrounded by spaces, and are separated by a -/// delimiter (e.g comma-separated) with optionally a trailing delimiter. -/// Braces and delimiters get discarded. -#[macro_export] -macro_rules! collection_trailing_sep { - ($opening_brace:expr, $elem:expr, $delimiter:expr, $closing_brace:expr, $min_indent:expr) => { - skip_first!( - $opening_brace, - skip_first!( - // We specifically allow space characters inside here, so that - // `[ ]` can be successfully parsed as an empty list, and then - // changed by the formatter back into `[]`. - // - // We don't allow newlines or comments in the middle of empty - // roc_collections because those are normally stored in an Expr, - // and there's no Expr in which to store them in an empty collection! - // - // We could change the AST to add extra storage specifically to - // support empty literals containing newlines or comments, but this - // does not seem worth even the tiniest regression in compiler performance. - zero_or_more!($crate::parser::ascii_char(b' ')), - skip_second!( - and!( - $crate::parser::trailing_sep_by0( - $delimiter, - $crate::blankspace::space0_around($elem, $min_indent) - ), - $crate::blankspace::space0($min_indent) - ), - $closing_brace - ) - ) - ) - }; -} - #[macro_export] macro_rules! collection_trailing_sep_e { ($opening_brace:expr, $elem:expr, $delimiter:expr, $closing_brace:expr, $min_indent:expr, $open_problem:expr, $space_problem:expr, $indent_problem:expr, $space_before:expr) => { From 6fab6e20f5d461d16accb081fe740cd02e627906 Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Sun, 14 Nov 2021 10:40:09 -0800 Subject: [PATCH 42/44] Fix clippy --- compiler/load/src/file.rs | 4 ++-- compiler/parse/src/module.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/load/src/file.rs b/compiler/load/src/file.rs index c733460053..17a370d49a 100644 --- a/compiler/load/src/file.rs +++ b/compiler/load/src/file.rs @@ -3236,7 +3236,7 @@ fn send_header_two<'a>( let extra = HeaderFor::PkgConfig { config_shorthand: shorthand, - platform_main_type: requires[0].value.clone(), + platform_main_type: requires[0].value, main_for_host, }; @@ -3419,7 +3419,7 @@ fn fabricate_pkg_config_module<'a>( app_module_id, packages: &[], provides, - requires: arena.alloc([header.requires.signature.clone()]), + requires: arena.alloc([header.requires.signature]), imports: header.imports.items, }; diff --git a/compiler/parse/src/module.rs b/compiler/parse/src/module.rs index 7a194fe073..f0d2200e49 100644 --- a/compiler/parse/src/module.rs +++ b/compiler/parse/src/module.rs @@ -811,7 +811,7 @@ fn imports_entry<'a>() -> impl Parser<'a, ImportsEntry<'a>, EImports> { )) ), |_arena, ((opt_shortname, module_name), opt_values): Temp<'a>| { - let exposed_values = opt_values.unwrap_or_else(|| Collection::empty()); + let exposed_values = opt_values.unwrap_or_else(Collection::empty); match opt_shortname { Some(shortname) => ImportsEntry::Package(shortname, module_name, exposed_values), From 29d355c4d6e8bf624bf304c809ffc85c1fdac8b9 Mon Sep 17 00:00:00 2001 From: Brian Carroll Date: Sun, 14 Nov 2021 19:06:04 +0000 Subject: [PATCH 43/44] Implement Expr::StructAtIndex for wasm dev backend --- compiler/gen_wasm/src/backend.rs | 27 +++++++++++- compiler/gen_wasm/src/storage.rs | 61 ++++++++++++++++++++++++++++ compiler/test_gen/src/gen_records.rs | 22 +++++----- 3 files changed, 97 insertions(+), 13 deletions(-) diff --git a/compiler/gen_wasm/src/backend.rs b/compiler/gen_wasm/src/backend.rs index 9093401626..aaffcecac9 100644 --- a/compiler/gen_wasm/src/backend.rs +++ b/compiler/gen_wasm/src/backend.rs @@ -22,8 +22,8 @@ use crate::wasm_module::{ LocalId, Signature, SymInfo, ValueType, }; use crate::{ - copy_memory, CopyMemoryConfig, Env, BUILTINS_IMPORT_MODULE_NAME, MEMORY_NAME, PTR_TYPE, - STACK_POINTER_GLOBAL_ID, STACK_POINTER_NAME, + copy_memory, CopyMemoryConfig, Env, BUILTINS_IMPORT_MODULE_NAME, MEMORY_NAME, PTR_SIZE, + PTR_TYPE, STACK_POINTER_GLOBAL_ID, STACK_POINTER_NAME, }; /// The memory address where the constants data will be loaded during module instantiation. @@ -540,6 +540,29 @@ impl<'a> WasmBackend<'a> { Expr::Struct(fields) => self.create_struct(sym, layout, fields), + Expr::StructAtIndex { + index, + field_layouts, + structure, + } => { + if let StoredValue::StackMemory { location, .. } = self.storage.get(structure) { + let (local_id, mut offset) = + location.local_and_offset(self.storage.stack_frame_pointer); + for field in field_layouts.iter().take(*index as usize) { + offset += field.stack_size(PTR_SIZE); + } + self.storage.copy_value_from_memory( + &mut self.code_builder, + *sym, + local_id, + offset, + ); + } else { + unreachable!("Unexpected storage for {:?}", structure) + } + Ok(()) + } + x => Err(format!("Expression is not yet implemented {:?}", x)), } } diff --git a/compiler/gen_wasm/src/storage.rs b/compiler/gen_wasm/src/storage.rs index f552d59c59..fbec6ff61a 100644 --- a/compiler/gen_wasm/src/storage.rs +++ b/compiler/gen_wasm/src/storage.rs @@ -319,6 +319,67 @@ impl<'a> Storage<'a> { } } + /// Generate code to copy a StoredValue from an arbitrary memory location + /// (defined by a pointer and offset). + pub fn copy_value_from_memory( + &mut self, + code_builder: &mut CodeBuilder, + to_symbol: Symbol, + from_ptr: LocalId, + from_offset: u32, + ) -> u32 { + let to_storage = self.get(&to_symbol).to_owned(); + match to_storage { + StoredValue::StackMemory { + location, + size, + alignment_bytes, + } => { + let (to_ptr, to_offset) = location.local_and_offset(self.stack_frame_pointer); + copy_memory( + code_builder, + CopyMemoryConfig { + from_ptr, + from_offset, + to_ptr, + to_offset, + size, + alignment_bytes, + }, + ); + size + } + + StoredValue::VirtualMachineStack { + value_type, size, .. + } + | StoredValue::Local { + value_type, size, .. + } => { + use crate::wasm_module::Align::*; + + code_builder.get_local(from_ptr); + match (value_type, size) { + (ValueType::I64, 8) => code_builder.i64_load(Bytes8, from_offset), + (ValueType::I32, 4) => code_builder.i32_load(Bytes4, from_offset), + (ValueType::I32, 2) => code_builder.i32_load16_s(Bytes2, from_offset), + (ValueType::I32, 1) => code_builder.i32_load8_s(Bytes1, from_offset), + (ValueType::F32, 4) => code_builder.f32_load(Bytes4, from_offset), + (ValueType::F64, 8) => code_builder.f64_load(Bytes8, from_offset), + _ => { + panic!("Cannot store {:?} with alignment of {:?}", value_type, size); + } + }; + + if let StoredValue::Local { local_id, .. } = to_storage { + code_builder.set_local(local_id); + } + + size + } + } + } + /// Generate code to copy from one StoredValue to another /// Copies the _entire_ value. For struct fields etc., see `copy_value_to_memory` pub fn clone_value( diff --git a/compiler/test_gen/src/gen_records.rs b/compiler/test_gen/src/gen_records.rs index 927daf0e18..1c6274e20b 100644 --- a/compiler/test_gen/src/gen_records.rs +++ b/compiler/test_gen/src/gen_records.rs @@ -11,7 +11,7 @@ use crate::helpers::wasm::assert_evals_to; use indoc::indoc; #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] fn basic_record() { assert_evals_to!( indoc!( @@ -45,7 +45,7 @@ fn basic_record() { } #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] fn f64_record() { assert_evals_to!( indoc!( @@ -137,7 +137,7 @@ fn fn_record() { } #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] fn def_record() { assert_evals_to!( indoc!( @@ -192,7 +192,7 @@ fn when_on_record() { } #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] fn when_record_with_guard_pattern() { assert_evals_to!( indoc!( @@ -207,7 +207,7 @@ fn when_record_with_guard_pattern() { } #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] fn let_with_record_pattern() { assert_evals_to!( indoc!( @@ -223,7 +223,7 @@ fn let_with_record_pattern() { } #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] fn record_guard_pattern() { assert_evals_to!( indoc!( @@ -239,7 +239,7 @@ fn record_guard_pattern() { } #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-wasm"))] fn twice_record_access() { assert_evals_to!( indoc!( @@ -254,7 +254,7 @@ fn twice_record_access() { ); } #[test] -#[cfg(any(feature = "gen-llvm", feature = "gen-dev", feature = "gen-dev"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-dev"))] fn empty_record() { assert_evals_to!( indoc!( @@ -873,7 +873,7 @@ fn update_single_element_record() { } #[test] -#[cfg(any(feature = "gen-llvm"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))] fn booleans_in_record() { assert_evals_to!( indoc!("{ x: 1 == 1, y: 1 == 1 }"), @@ -908,7 +908,7 @@ fn alignment_in_record() { } #[test] -#[cfg(any(feature = "gen-llvm"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))] fn blue_and_present() { assert_evals_to!( indoc!( @@ -927,7 +927,7 @@ fn blue_and_present() { } #[test] -#[cfg(any(feature = "gen-llvm"))] +#[cfg(any(feature = "gen-llvm", feature = "gen-wasm"))] fn blue_and_absent() { assert_evals_to!( indoc!( From a384042c59eba9ad269e61e8cfff7376eaa5958c Mon Sep 17 00:00:00 2001 From: Joshua Warner Date: Sun, 14 Nov 2021 13:25:17 -0800 Subject: [PATCH 44/44] add handling for ListEnd errors --- compiler/reporting/src/error/parse.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/reporting/src/error/parse.rs b/compiler/reporting/src/error/parse.rs index 163a2b118b..505e41a736 100644 --- a/compiler/reporting/src/error/parse.rs +++ b/compiler/reporting/src/error/parse.rs @@ -3093,6 +3093,7 @@ fn to_provides_report<'a>( use roc_parse::parser::EProvides; match *parse_problem { + EProvides::ListEnd(row, col) | // TODO: give this its own error message EProvides::Identifier(row, col) => { let surroundings = Region::from_rows_cols(start_row, start_col, row, col); let region = Region::from_row_col(row, col); @@ -3158,6 +3159,7 @@ fn to_exposes_report<'a>( use roc_parse::parser::EExposes; match *parse_problem { + EExposes::ListEnd(row, col) | // TODO: give this its own error message EExposes::Identifier(row, col) => { let surroundings = Region::from_rows_cols(start_row, start_col, row, col); let region = Region::from_row_col(row, col);