From f45d89270b843e398c8c908565cf84adbf4d2db4 Mon Sep 17 00:00:00 2001 From: tarjei Date: Tue, 18 May 2021 22:59:55 +0200 Subject: [PATCH 1/6] Implement basic List.drop that satisfies tests Issues with uniqueness and copying which leaks memory obviously needs to be fixed. --- compiler/builtins/bitcode/src/list.zig | 27 +++++++++++++++++++ compiler/builtins/bitcode/src/main.zig | 1 + compiler/builtins/src/bitcode.rs | 1 + compiler/builtins/src/std.rs | 7 +++++ compiler/can/src/builtins.rs | 23 ++++++++++++++++ compiler/gen/src/llvm/build.rs | 24 ++++++++++++++--- compiler/gen/src/llvm/build_list.rs | 36 ++++++++++++++++++++++++++ compiler/module/src/low_level.rs | 1 + compiler/module/src/symbol.rs | 1 + compiler/mono/src/borrow.rs | 1 + compiler/test_gen/src/gen_list.rs | 11 ++++++++ 11 files changed, 130 insertions(+), 3 deletions(-) diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 1791a8f61b..14814c3835 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -665,6 +665,33 @@ pub fn listAppend(list: RocList, alignment: usize, element: Opaque, element_widt return output; } +pub fn listDrop(list: RocList, alignment: usize, element_width: usize, count: usize) callconv(.C) RocList { + const size = list.len(); + if (size <= count) { + return RocList.empty(); + } + if (list.bytes) |source_ptr| { + var i: usize = 0; + + const output = RocList.allocate(std.heap.c_allocator, alignment, size - count, element_width); + const target_ptr = output.bytes orelse unreachable; + + while (i < size - count) : (i += 1) { + @memcpy(target_ptr + (i * element_width), source_ptr + ((i + count) * element_width), element_width); + } + + // if (list.isUnique()) { + // std.heap.c_allocator.free(source_ptr); + // } else { + // utils.decref(std.heap.c_allocator, alignment, list.bytes, size * element_width); + // } + + return output; + } else { + return RocList.empty(); + } +} + pub fn listRange(width: utils.IntWidth, low: Opaque, high: Opaque) callconv(.C) RocList { const allocator = std.heap.c_allocator; const IntWidth = utils.IntWidth; diff --git a/compiler/builtins/bitcode/src/main.zig b/compiler/builtins/bitcode/src/main.zig index 18db41131c..6eb689bd66 100644 --- a/compiler/builtins/bitcode/src/main.zig +++ b/compiler/builtins/bitcode/src/main.zig @@ -25,6 +25,7 @@ comptime { exportListFn(list.listReverse, "reverse"); exportListFn(list.listSortWith, "sort_with"); exportListFn(list.listConcat, "concat"); + exportListFn(list.listDrop, "drop"); } // Dict Module diff --git a/compiler/builtins/src/bitcode.rs b/compiler/builtins/src/bitcode.rs index 2032e13700..7c3d82dc4d 100644 --- a/compiler/builtins/src/bitcode.rs +++ b/compiler/builtins/src/bitcode.rs @@ -76,6 +76,7 @@ pub const LIST_WALK_BACKWARDS: &str = "roc_builtins.list.walk_backwards"; pub const LIST_CONTAINS: &str = "roc_builtins.list.contains"; pub const LIST_REPEAT: &str = "roc_builtins.list.repeat"; pub const LIST_APPEND: &str = "roc_builtins.list.append"; +pub const LIST_DROP: &str = "roc_builtins.list.drop"; pub const LIST_SINGLE: &str = "roc_builtins.list.single"; pub const LIST_JOIN: &str = "roc_builtins.list.join"; pub const LIST_RANGE: &str = "roc_builtins.list.range"; diff --git a/compiler/builtins/src/std.rs b/compiler/builtins/src/std.rs index c0d8f9b39e..25d057cc81 100644 --- a/compiler/builtins/src/std.rs +++ b/compiler/builtins/src/std.rs @@ -848,6 +848,13 @@ pub fn types() -> MutMap { Box::new(list_type(flex(TVAR1))), ); + // drop : List elem, Nat -> List elem + add_top_level_function_type!( + Symbol::LIST_DROP, + vec![list_type(flex(TVAR1)), nat_type()], + Box::new(list_type(flex(TVAR1))), + ); + // prepend : List elem, elem -> List elem add_top_level_function_type!( Symbol::LIST_PREPEND, diff --git a/compiler/can/src/builtins.rs b/compiler/can/src/builtins.rs index edd6bb49d2..7283316ba9 100644 --- a/compiler/can/src/builtins.rs +++ b/compiler/can/src/builtins.rs @@ -84,6 +84,7 @@ pub fn builtin_defs_map(symbol: Symbol, var_store: &mut VarStore) -> Option LIST_MAP => list_map, LIST_MAP2 => list_map2, LIST_MAP3 => list_map3, + LIST_DROP => list_drop, LIST_MAP_WITH_INDEX => list_map_with_index, LIST_KEEP_IF => list_keep_if, LIST_KEEP_OKS => list_keep_oks, @@ -1881,6 +1882,28 @@ fn list_set(symbol: Symbol, var_store: &mut VarStore) -> Def { list_ret_var, ) } +/// List.drop : List elem, Nat -> List elem +fn list_drop(symbol: Symbol, var_store: &mut VarStore) -> Def { + let list_var = var_store.fresh(); + let index_var = var_store.fresh(); + + let body = RunLowLevel { + op: LowLevel::ListDrop, + args: vec![ + (list_var, Var(Symbol::ARG_1)), + (index_var, Var(Symbol::ARG_2)), + ], + ret_var: list_var, + }; + + defn( + symbol, + vec![(list_var, Symbol::ARG_1), (index_var, Symbol::ARG_2)], + var_store, + body, + list_var, + ) +} /// List.append : List elem, elem -> List elem fn list_append(symbol: Symbol, var_store: &mut VarStore) -> Def { diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs index ed0d0c1123..a56e6d7213 100644 --- a/compiler/gen/src/llvm/build.rs +++ b/compiler/gen/src/llvm/build.rs @@ -6,9 +6,9 @@ use crate::llvm::build_dict::{ use crate::llvm::build_hash::generic_hash; use crate::llvm::build_list::{ allocate_list, empty_list, empty_polymorphic_list, list_append, list_concat, list_contains, - list_get_unsafe, list_join, list_keep_errs, list_keep_if, list_keep_oks, list_len, list_map, - list_map2, list_map3, list_map_with_index, list_prepend, list_range, list_repeat, list_reverse, - list_set, list_single, list_sort_with, list_walk_help, + list_drop, list_get_unsafe, list_join, list_keep_errs, list_keep_if, list_keep_oks, list_len, + list_map, list_map2, list_map3, list_map_with_index, list_prepend, list_range, list_repeat, + list_reverse, list_set, list_single, list_sort_with, list_walk_help, }; use crate::llvm::build_str::{ empty_str, str_concat, str_count_graphemes, str_ends_with, str_from_float, str_from_int, @@ -3883,6 +3883,24 @@ fn run_low_level<'a, 'ctx, 'env>( list_append(env, inplace, original_wrapper, elem, elem_layout) } + ListDrop => { + // List.drop : List elem, Nat -> List elem + debug_assert_eq!(args.len(), 2); + + let (list, list_layout) = load_symbol_and_layout(scope, &args[0]); + let original_wrapper = list.into_struct_value(); + + let count = load_symbol(scope, &args[1]); + let inplace = get_inplace_from_layout(layout); + + list_drop( + env, + inplace, + original_wrapper, + count.into_int_value(), + list_layout, + ) + } ListPrepend => { // List.prepend : List elem, elem -> List elem debug_assert_eq!(args.len(), 2); diff --git a/compiler/gen/src/llvm/build_list.rs b/compiler/gen/src/llvm/build_list.rs index 6634c51862..5645985d4d 100644 --- a/compiler/gen/src/llvm/build_list.rs +++ b/compiler/gen/src/llvm/build_list.rs @@ -322,6 +322,42 @@ pub fn list_append<'a, 'ctx, 'env>( ) } +/// List.drop : List elem, Nat -> List elem +pub fn list_drop<'a, 'ctx, 'env>( + env: &Env<'a, 'ctx, 'env>, + _inplace: InPlace, + original_wrapper: StructValue<'ctx>, + count: IntValue<'ctx>, + list_layout: &Layout<'a>, +) -> BasicValueEnum<'ctx> { + let (_, element_layout) = match *list_layout { + Layout::Builtin(Builtin::EmptyList) => ( + InPlace::InPlace, + // this pointer will never actually be dereferenced + Layout::Builtin(Builtin::Int64), + ), + Layout::Builtin(Builtin::List(memory_mode, elem_layout)) => ( + match memory_mode { + MemoryMode::Unique => InPlace::InPlace, + MemoryMode::Refcounted => InPlace::Clone, + }, + *elem_layout, + ), + + _ => unreachable!("Invalid layout {:?} in List.drop", list_layout), + }; + call_bitcode_fn_returns_list( + env, + &[ + pass_list_as_i128(env, original_wrapper.into()), + alignment_intvalue(env, &element_layout), + layout_width(env, &element_layout), + count.into(), + ], + &bitcode::LIST_DROP, + ) +} + /// List.set : List elem, Int, elem -> List elem pub fn list_set<'a, 'ctx, 'env>( parent: FunctionValue<'ctx>, diff --git a/compiler/module/src/low_level.rs b/compiler/module/src/low_level.rs index cf6db1f394..65c8f93084 100644 --- a/compiler/module/src/low_level.rs +++ b/compiler/module/src/low_level.rs @@ -39,6 +39,7 @@ pub enum LowLevel { ListKeepOks, ListKeepErrs, ListSortWith, + ListDrop, DictSize, DictEmpty, DictInsert, diff --git a/compiler/module/src/symbol.rs b/compiler/module/src/symbol.rs index ad79d30e7d..095fa24529 100644 --- a/compiler/module/src/symbol.rs +++ b/compiler/module/src/symbol.rs @@ -924,6 +924,7 @@ define_builtins! { 29 LIST_WALK_UNTIL: "walkUntil" 30 LIST_RANGE: "range" 31 LIST_SORT_WITH: "sortWith" + 32 LIST_DROP: "drop" } 5 RESULT: "Result" => { 0 RESULT_RESULT: "Result" imported // the Result.Result type alias diff --git a/compiler/mono/src/borrow.rs b/compiler/mono/src/borrow.rs index 2657223d2e..de98da21dc 100644 --- a/compiler/mono/src/borrow.rs +++ b/compiler/mono/src/borrow.rs @@ -665,6 +665,7 @@ pub fn lowlevel_borrow_signature(arena: &Bump, op: LowLevel) -> &[bool] { // TODO when we have lists with capacity (if ever) // List.append should own its first argument ListAppend => arena.alloc_slice_copy(&[owned, owned]), + ListDrop => arena.alloc_slice_copy(&[owned, irrelevant]), Eq | NotEq => arena.alloc_slice_copy(&[borrowed, borrowed]), diff --git a/compiler/test_gen/src/gen_list.rs b/compiler/test_gen/src/gen_list.rs index 124f719a6b..5e9d02b031 100644 --- a/compiler/test_gen/src/gen_list.rs +++ b/compiler/test_gen/src/gen_list.rs @@ -147,6 +147,17 @@ fn list_append() { ); } +#[test] +fn list_drop() { + assert_evals_to!( + "List.drop [1,2,3] 2", + RocList::from_slice(&[3]), + RocList + ); + assert_evals_to!("List.drop [] 1", RocList::from_slice(&[]), RocList); + assert_evals_to!("List.drop [1,2] 5", RocList::from_slice(&[]), RocList); +} + #[test] fn list_append_to_empty_list() { assert_evals_to!("List.append [] 3", RocList::from_slice(&[3]), RocList); From ae6cd1ca4e78fcf28dbf2386cce930111f15e5b0 Mon Sep 17 00:00:00 2001 From: Brendan Hansknecht Date: Thu, 20 May 2021 20:45:06 -0700 Subject: [PATCH 2/6] Update test since frontend doesn't stack overflow anymore --- compiler/gen_dev/tests/gen_num.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compiler/gen_dev/tests/gen_num.rs b/compiler/gen_dev/tests/gen_num.rs index 80324a3f69..68b2dca31c 100644 --- a/compiler/gen_dev/tests/gen_num.rs +++ b/compiler/gen_dev/tests/gen_num.rs @@ -128,9 +128,7 @@ mod gen_num { af = 31 ag = 32 - # This can't be one line because it causes a stack overflow in the frontend :( - tmp = a + b + c + d + e + f + g + h + i + j + k + l + m + n + o + p + q - tmp + r + s + t + u + v + w + x + y + z + aa + ab + ac + ad + ae + af + ag + a + b + c + d + e + f + g + h + i + j + k + l + m + n + o + p + q + r + s + t + u + v + w + x + y + z + aa + ab + ac + ad + ae + af + ag "# ), 528, From 85e5b0ff82f5bdc531b8c70fa9181e7f8e53b581 Mon Sep 17 00:00:00 2001 From: tarjei Date: Fri, 21 May 2021 21:53:55 +0200 Subject: [PATCH 3/6] Fix most of deref logic Still doesn't handle empty lists properly. --- compiler/build/src/link.rs | 4 ++-- compiler/builtins/bitcode/src/list.zig | 23 +++++++++++------------ compiler/gen/src/llvm/build.rs | 19 +++++++++++-------- compiler/gen/src/llvm/build_list.rs | 22 ++++------------------ 4 files changed, 28 insertions(+), 40 deletions(-) diff --git a/compiler/build/src/link.rs b/compiler/build/src/link.rs index 95983e1a8d..e2975d839b 100644 --- a/compiler/build/src/link.rs +++ b/compiler/build/src/link.rs @@ -466,8 +466,8 @@ fn link_macos( "-lc++", // "-lc++abi", // "-lunwind", // TODO will eventually need this, see https://github.com/rtfeldman/roc/pull/554#discussion_r496370840 - "-framework", - "Security", // This "-framework Security" arg is needed for the `rand` crate in examples/cli + //"-framework", + //"Security", // This "-framework Security" arg is needed for the `rand` crate in examples/cli // Output "-o", output_path.to_str().unwrap(), // app diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 14814c3835..380e1b4d1d 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -665,26 +665,25 @@ pub fn listAppend(list: RocList, alignment: usize, element: Opaque, element_widt return output; } -pub fn listDrop(list: RocList, alignment: usize, element_width: usize, count: usize) callconv(.C) RocList { +pub fn listDrop(list: RocList, alignment: usize, element_width: usize, drop_count: usize, dec: Dec) callconv(.C) RocList { const size = list.len(); - if (size <= count) { + const keep_count = size - drop_count; + if (size <= drop_count) { return RocList.empty(); } if (list.bytes) |source_ptr| { - var i: usize = 0; - - const output = RocList.allocate(std.heap.c_allocator, alignment, size - count, element_width); + const output = RocList.allocate(std.heap.c_allocator, alignment, keep_count, element_width); const target_ptr = output.bytes orelse unreachable; - while (i < size - count) : (i += 1) { - @memcpy(target_ptr + (i * element_width), source_ptr + ((i + count) * element_width), element_width); + @memcpy(target_ptr, source_ptr + drop_count * element_width, keep_count * element_width); + + var i: usize = 0; + while (i < drop_count) : (i += 1) { + const element = source_ptr + i * element_width; + dec(element); } - // if (list.isUnique()) { - // std.heap.c_allocator.free(source_ptr); - // } else { - // utils.decref(std.heap.c_allocator, alignment, list.bytes, size * element_width); - // } + utils.decref(std.heap.c_allocator, alignment, list.bytes, size * element_width); return output; } else { diff --git a/compiler/gen/src/llvm/build.rs b/compiler/gen/src/llvm/build.rs index a56e6d7213..cf0271db60 100644 --- a/compiler/gen/src/llvm/build.rs +++ b/compiler/gen/src/llvm/build.rs @@ -3891,15 +3891,18 @@ fn run_low_level<'a, 'ctx, 'env>( let original_wrapper = list.into_struct_value(); let count = load_symbol(scope, &args[1]); - let inplace = get_inplace_from_layout(layout); - list_drop( - env, - inplace, - original_wrapper, - count.into_int_value(), - list_layout, - ) + match list_layout { + Layout::Builtin(Builtin::EmptyList) => empty_list(env), + Layout::Builtin(Builtin::List(_, element_layout)) => list_drop( + env, + layout_ids, + original_wrapper, + count.into_int_value(), + element_layout, + ), + _ => unreachable!("Invalid layout {:?} in List.drop", list_layout), + } } ListPrepend => { // List.prepend : List elem, elem -> List elem diff --git a/compiler/gen/src/llvm/build_list.rs b/compiler/gen/src/llvm/build_list.rs index 5645985d4d..1637f33c8a 100644 --- a/compiler/gen/src/llvm/build_list.rs +++ b/compiler/gen/src/llvm/build_list.rs @@ -325,27 +325,12 @@ pub fn list_append<'a, 'ctx, 'env>( /// List.drop : List elem, Nat -> List elem pub fn list_drop<'a, 'ctx, 'env>( env: &Env<'a, 'ctx, 'env>, - _inplace: InPlace, + layout_ids: &mut LayoutIds<'a>, original_wrapper: StructValue<'ctx>, count: IntValue<'ctx>, - list_layout: &Layout<'a>, + element_layout: &Layout<'a>, ) -> BasicValueEnum<'ctx> { - let (_, element_layout) = match *list_layout { - Layout::Builtin(Builtin::EmptyList) => ( - InPlace::InPlace, - // this pointer will never actually be dereferenced - Layout::Builtin(Builtin::Int64), - ), - Layout::Builtin(Builtin::List(memory_mode, elem_layout)) => ( - match memory_mode { - MemoryMode::Unique => InPlace::InPlace, - MemoryMode::Refcounted => InPlace::Clone, - }, - *elem_layout, - ), - - _ => unreachable!("Invalid layout {:?} in List.drop", list_layout), - }; + let dec_element_fn = build_dec_wrapper(env, layout_ids, &element_layout); call_bitcode_fn_returns_list( env, &[ @@ -353,6 +338,7 @@ pub fn list_drop<'a, 'ctx, 'env>( alignment_intvalue(env, &element_layout), layout_width(env, &element_layout), count.into(), + dec_element_fn.as_global_value().as_pointer_value().into(), ], &bitcode::LIST_DROP, ) From e062bdaad8b5aaedfa2014e393c35cbeeccbe308 Mon Sep 17 00:00:00 2001 From: tarjei Date: Sat, 22 May 2021 00:54:03 +0200 Subject: [PATCH 4/6] Fix dec ref for empty list --- compiler/builtins/bitcode/src/list.zig | 32 ++++++++++++++++---------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 380e1b4d1d..771d79f01d 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -665,24 +665,32 @@ pub fn listAppend(list: RocList, alignment: usize, element: Opaque, element_widt return output; } -pub fn listDrop(list: RocList, alignment: usize, element_width: usize, drop_count: usize, dec: Dec) callconv(.C) RocList { - const size = list.len(); - const keep_count = size - drop_count; - if (size <= drop_count) { - return RocList.empty(); - } +pub fn listDrop( + list: RocList, + alignment: usize, + element_width: usize, + drop_count: usize, + dec: Dec, +) callconv(.C) RocList { if (list.bytes) |source_ptr| { + const size = list.len(); + const keep_count = size - drop_count; + + var i: usize = 0; + while (i < std.math.min(drop_count, size)) : (i += 1) { + const element = source_ptr + i * element_width; + dec(element); + } + + if (drop_count >= size) { + return RocList.empty(); + } + const output = RocList.allocate(std.heap.c_allocator, alignment, keep_count, element_width); const target_ptr = output.bytes orelse unreachable; @memcpy(target_ptr, source_ptr + drop_count * element_width, keep_count * element_width); - var i: usize = 0; - while (i < drop_count) : (i += 1) { - const element = source_ptr + i * element_width; - dec(element); - } - utils.decref(std.heap.c_allocator, alignment, list.bytes, size * element_width); return output; From 46a856742717ce2b43aa5ec9481ae497bcedbb7e Mon Sep 17 00:00:00 2001 From: tarjei Date: Sat, 22 May 2021 00:59:13 +0200 Subject: [PATCH 5/6] Uncomment rust link stuff --- compiler/build/src/link.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/build/src/link.rs b/compiler/build/src/link.rs index e2975d839b..95983e1a8d 100644 --- a/compiler/build/src/link.rs +++ b/compiler/build/src/link.rs @@ -466,8 +466,8 @@ fn link_macos( "-lc++", // "-lc++abi", // "-lunwind", // TODO will eventually need this, see https://github.com/rtfeldman/roc/pull/554#discussion_r496370840 - //"-framework", - //"Security", // This "-framework Security" arg is needed for the `rand` crate in examples/cli + "-framework", + "Security", // This "-framework Security" arg is needed for the `rand` crate in examples/cli // Output "-o", output_path.to_str().unwrap(), // app From 8666a2ad584702e1063a651c1144fb7caf062058 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Sat, 22 May 2021 14:48:40 -0400 Subject: [PATCH 6/6] Avoid recomputing loop termination condition LLVM might take care of this for us, but just to be safe! --- compiler/builtins/bitcode/src/list.zig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/builtins/bitcode/src/list.zig b/compiler/builtins/bitcode/src/list.zig index 771d79f01d..bd31d71951 100644 --- a/compiler/builtins/bitcode/src/list.zig +++ b/compiler/builtins/bitcode/src/list.zig @@ -677,7 +677,9 @@ pub fn listDrop( const keep_count = size - drop_count; var i: usize = 0; - while (i < std.math.min(drop_count, size)) : (i += 1) { + const iterations = std.math.min(drop_count, size); + + while (i < iterations) : (i += 1) { const element = source_ptr + i * element_width; dec(element); }